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.

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

Fix for unyt 2.9.3 change in behaviour #17

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

jchelly
Copy link
Collaborator

@jchelly jchelly commented Apr 3, 2023

For each halo we store a dict of unyt_quantities with the ID, position, search radius etc of the halo. This includes a flag to indicate if a halo has been processed.

In several places SOAP checks if the flag is set by comparing with 0. In unyt 2.9.3 the comparison always returns False and causes us to read zero cells from the snapshot and crash (see #6). This pull request modifies the code so that it's not sensitive to the changed behaviour.

The result of comparison of an element of a dimensionless
unyt_array to an integer changed in unyt 2.9.3, so avoid
doing that!
@JBorrow
Copy link
Member

JBorrow commented Apr 3, 2023

I think that this is a bug in unyt. But we should check with unyt 3.0.0

@MatthieuSchaller
Copy link
Member

It is. But we agreed we should program defensively against their stuff....

Also we need this to work now.

@JBorrow
Copy link
Member

JBorrow commented Apr 4, 2023

I mean it is fixed in 2.9.4 right?

@JBorrow
Copy link
Member

JBorrow commented Apr 4, 2023

However, I think actually doing this conversion to values when comparing against zero is probably a good practice anyway.

@jchelly jchelly marked this pull request as ready for review April 6, 2023 11:18
Copy link
Member

@MatthieuSchaller MatthieuSchaller left a comment

Choose a reason for hiding this comment

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

All good for me. Better safe than sorry even if the code gets fixed upstream.

@MatthieuSchaller MatthieuSchaller merged commit 170fa20 into master Apr 6, 2023
@MatthieuSchaller MatthieuSchaller deleted the unyt_compare_zero_fix branch April 6, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants