Merged
Conversation
Temporal indexing is left separate from spatial indexing due to the fact that ti was initialized differently from spatial indices on L131 of particleset.py (ti = -1, xi,yi,zi=0). To stick with this convention and to avoid an unforeseen consquences of merging spatial and temporal indexing, I've opted to kep them separate with ei=0 initially and ti=-1 initially. ravel_index/unravel_index methods have been added to the Field class. In this commit, some tests aren't passing as it appears some tests attempt to check the values of the old xi,yi,zi .
for more information, see https://pre-commit.ci
This was
linked to
issues
Feb 24, 2025
Contributor
Author
|
It appears there's a good number of notebooks to go through and update.. |
Using the new element index unravel method
Member
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Nice, clean implementation @fluidnumerics-joe! A few comments/suggestions below
Member
There was only one notebook that failed; I fixed it in 7c5531d |
…rcels into 1874-ravel-unravel-index
VeckoTheGecko
approved these changes
Feb 25, 2025
Contributor
VeckoTheGecko
left a comment
VeckoTheGecko
left a comment
There was a problem hiding this comment.
Looks good! After the open questions, and a merge with v4-dev I think this will be good to go :)
for more information, see https://pre-commit.ci
…rcels into 1874-ravel-unravel-index
2 tasks
erikvansebille
approved these changes
Feb 26, 2025
Member
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Looks all good now, but we may want to also merge in #1881 (adding ti to the ravelled index too)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mainfor v3 changes,v4-devfor v4 changes)Notes
ParticleSetnow uses indicesei,tiin place ofzi,yi,xi,ti. The time index is kept separate since it appears that the initialization fortiis a bit different than for spatial indices; I wasn't convinced that combining spatial and temporal indexing wouldn't have unforeseen consequences.Field.ravel_indexandField.unravel_indexhave been added to map betweeneiand(zi,yi,xi)_index_search*methods have been updated to ravel and unravel indices where needed.(zi,yi,xi)and instead rely onunravel_indexwhere needed.