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

BUG: Make sure fields are read in the order they appear in the file #4907

Merged
merged 3 commits into from
May 21, 2024

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented May 17, 2024

PR Summary

This fixes #4880.

The issue was that the reader would assume fields were passed in the order they appeared on file, so that depending on the order in which one would load the data, we'd get different results.

@cphyc cphyc added bug code frontends Things related to specific frontends labels May 17, 2024
@matthewturk
Copy link
Member

This looks like it might fix bugs ... were there any?

@cphyc
Copy link
Member Author

cphyc commented May 17, 2024

@matthewturk yes, this fixes the bug reported in #4880 (which IMO is a major one).

Essentially, when yt detects field dependences, it creates a list of on-disk fields in an arbitrary order. This then gets passed to the reader, which (wrongly) was assuming the fields were passed in the order the appear in the file.

In #4880, yt tries to read density, metallicity, pressure but the fields appear on-disk in the order density, pressure, metallicity. As a consequence, pressure and metallicity end up being swapped. This PR fixes this by ordering the fields in the order they appear on disk and voilà.

@neutrinoceros neutrinoceros added this to the 4.4.0 milestone May 18, 2024
@neutrinoceros
Copy link
Member

Hum. This looks simple enough but I'm not comfortable to merge a fix for a major bug without at least an attempt at automated testing. It probably not possible to prove the software is always correct, but a simple regression test seems feasible ?

@cphyc
Copy link
Member Author

cphyc commented May 18, 2024

I'll give it a try, but so far I only was able to trigger it with a somewhat massive simulation (few 10GiB).

@matthewturk
Copy link
Member

I think the likelihood of a problem is low enough that this should go in. We do similar logic elsewhere, and this seems absolutely correct.

matthewturk
matthewturk previously approved these changes May 18, 2024
@neutrinoceros
Copy link
Member

Fair enough. @cphyc, suit yourself !

@neutrinoceros
Copy link
Member

(to be clear, I'm not worried that the patch is incorrect, I'm worried that a similar bug might creep back in at some later point in the future and we have to pay the full price of figuring it out again)

@cphyc
Copy link
Member Author

cphyc commented May 18, 2024

I've added a test (972ccec) and checked locally it correctly fails against main (2880d15).

FYI, I've added a nose test rather than a pytest one because it appears that pytest-based tests don't use actual data…

@cphyc cphyc changed the title Make sure fields are read in the order they appear in the file BUG: Make sure fields are read in the order they appear in the file May 20, 2024
@@ -644,3 +644,42 @@ def test_print_stats():
ds.print_stats()

# FIXME #3197: use `capsys` with pytest to make sure the print_stats function works as intended


def _dummy_field(field, data):
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to move this inside the test function for clarity

neutrinoceros
neutrinoceros previously approved these changes May 20, 2024
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I checked the test fails on main and pass with this branch, thanks a lot ! Feel free to ignore my minor remark and merge if you will, otherwise I'm happy to approve again later today.

@neutrinoceros neutrinoceros merged commit 36a897f into yt-project:main May 21, 2024
13 checks passed
@cphyc cphyc deleted the bugfix/4880 branch May 21, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code frontends Things related to specific frontends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: some RAMSES fields return different values depending on order of operations
3 participants