[bugfix] Fixing issues with particles #2338
Merged
Conversation
This file contains 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
|
Thank you for this. Tomorrow on the triage call let's go over this, but it
seems quite important.
…On Tue, Sep 24, 2019 at 8:38 PM John ZuHone ***@***.***> wrote:
This is one of those PRs which basically led one down a rabbit hole,
finding several issues along the way.
1. The DiskSelector was computing selections incorrectly for SPH
particles.
2. The relative_particle_position and relative_particle_velocity
fields were using modify_reference_frame to compute the relative
fields. This takes a normal vector and then returns the relative
coordinates in a rotated coordinate system, in which the new normal vector
is the z-axis. However, the fields for particle positions in spherical and
cylindrical coordinates (such as radius, etc.) were assuming the normal
field parameter was the original one instead and using the relative fields,
which gave incorrect results. I'm changing here the way the relative fields
are computed, consistent with the way we handle relative fields from grid
datasets, by only subtracting the center off. As far as I can tell, this
only occurs in particle_fields.py.
3. For some reason merges with master did not get the fixes from PR
#2043 <#2043> into the yt-4.0
branch, this adds them back in.
Open to suggestions about new tests.
PR Checklist
- Code passes flake8 checker
- Adds a test for any bugs fixed.
------------------------------
You can view, comment on, or merge this pull request online at:
#2338
Commit Summary
- These need to have their units converted to code_length first
- Fix bad merge from master
- this needs to be the same as in select_point directly above
- Allow this to be used with particles also
- Don't use modify_reference_frame in here
File Changes
- *M* yt/fields/angular_momentum.py
<https://github.com/yt-project/yt/pull/2338/files#diff-0> (8)
- *M* yt/fields/particle_fields.py
<https://github.com/yt-project/yt/pull/2338/files#diff-1> (36)
- *M* yt/fields/tests/test_angular_momentum.py
<https://github.com/yt-project/yt/pull/2338/files#diff-2> (2)
- *M* yt/geometry/selection_routines.pyx
<https://github.com/yt-project/yt/pull/2338/files#diff-3> (2)
- *M* yt/utilities/lib/misc_utilities.pyx
<https://github.com/yt-project/yt/pull/2338/files#diff-4> (19)
Patch Links:
- https://github.com/yt-project/yt/pull/2338.patch
- https://github.com/yt-project/yt/pull/2338.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2338?email_source=notifications&email_token=AAAVXO3NRCVZKYUFJBTP653QLK6J5A5CNFSM4I2GSWFKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HNO4VLA>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAVXOZCAMOZSSUM3Q3SPKLQLK6J5ANCNFSM4I2GSWFA>
.
|
matthewturk
approved these changes
Sep 27, 2019
| @@ -1230,7 +1230,7 @@ cdef class DiskSelector(SelectorObject): | |||
| h = d = 0 | |||
| for i in range(3): | |||
| temp = self.periodic_difference(pos[i], self.center[i], i) | |||
| h += pos[i] * self.norm_vec[i] | |||
| h += temp * self.norm_vec[i] | |||
| @@ -568,7 +568,8 @@ def get_box_grids_below_level( | |||
| @cython.cdivision(True) | |||
| @cython.boundscheck(False) | |||
| @cython.wraparound(False) | |||
| def obtain_position_vector(data): | |||
| def obtain_position_vector( | |||
| data, field_names = ('x', 'y', 'z')): | |||
There was a problem hiding this comment.
should maybe add a note that this may result in an ambiguous access
|
Some of the angular momentum stuff is included in #2172 but this is a set of great catches. Thanks. |
munkm
approved these changes
Oct 1, 2019
| @@ -568,7 +568,8 @@ def get_box_grids_below_level( | |||
| @cython.cdivision(True) | |||
| @cython.boundscheck(False) | |||
| @cython.wraparound(False) | |||
| def obtain_position_vector(data): | |||
| def obtain_position_vector( | |||
| data, field_names = ('x', 'y', 'z')): | |||
|
@jzuhone is going to add a couple tests to this and then it'll be ready to go. |
|
After the tests pass, this is ready to go. I think disk objects need to be added to the AMR and SPH answer tests, which would have to come in another PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is one of those PRs which basically led one down a rabbit hole, finding several issues along the way.
DiskSelectorwas computing selections incorrectly for SPH particles.relative_particle_positionandrelative_particle_velocityfields were usingmodify_reference_frameto compute the relative fields. This takes a normal vector and then returns the relative coordinates in a rotated coordinate system, in which the new normal vector is the z-axis. However, the fields for particle positions in spherical and cylindrical coordinates (such as radius, etc.) were assuming thenormalfield parameter was the original one instead and using the relative fields, which gave incorrect results. I'm changing here the way the relative fields are computed, consistent with the way we handle relative fields from grid datasets, by only subtracting the center off. As far as I can tell, this only occurs inparticle_fields.py.yt-4.0branch, this adds them back in.Open to suggestions about new tests.
PR Checklist