Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure positions are in code_length in get_radius function. #4091

Merged
merged 3 commits into from
Aug 25, 2022
Merged

Make sure positions are in code_length in get_radius function. #4091

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a234f1
Make sure positions are in code_length in get_radius function.
brittonsmith Aug 22, 2022
6239c55
Only convert units if we need to.
brittonsmith Aug 22, 2022
98cab40
Make a copy when converting position units.
brittonsmith Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion yt/fields/field_functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ def get_radius(data, field_prefix, ftype):
if any(data.ds.periodicity):
rdw = radius2.v
for i, ax in enumerate("xyz"):
pos = data[ftype, f"{field_prefix}{ax}"]
if str(pos.units) != "code_length":
pos.convert_to_units("code_length")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now we're converting in place, changing the data outputted by ytree. Are we certain this isn't going to cause problems in ytree ?
Also I don't think I understand why it can't be changed in ytree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we can guarantee that every yt frontend is reading particle positions in "code_length", the problem will not be limited to ytree. It makes me nervous that we are not checking units before operating on memory views.

I'm happy to switch it back to making a copy. Would the solution I proposed above be better? That is, should we check the units of the position field using the field info container and the convert the other variables to that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we can guarantee that every yt frontend is reading particle positions in "code_length", the problem will not be limited to ytree. It makes me nervous that we are not checking units before operating on memory views.

ok that makes sense.

I'm happy to switch it back to making a copy.

I think it'd be the best option because it's the most conservative approach for these edge cases where we're not sure what to expect, and also an incentive for frontend development to use "code_lenght" more uniformly since it allows to use the fast track where no copy is performed.

Would the solution I proposed above be better? That is, should we check the units of the position field using the field info container and the convert the other variables to that?

I guess I don't have strong opinions there. I think the current solution is already good in that it is probably the simplest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with sticking with the simplest solution for now. I'll switch thing back to using a copy and forgo any further optimization.

np.subtract(
data[ftype, f"{field_prefix}{ax}"].d,
pos.d,
center[i].d,
r,
)
Expand Down