-
Notifications
You must be signed in to change notification settings - Fork 280
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
Conversation
This looks like it might fix bugs ... were there any? |
@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à. |
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 ? |
I'll give it a try, but so far I only was able to trigger it with a somewhat massive simulation (few 10GiB). |
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. |
Fair enough. @cphyc, suit yourself ! |
(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) |
@@ -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): |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
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.