search

yt-project / yt

Main yt repository
http://yt-project.org
Other
454 stars 272 forks source link

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

Closed cphyc closed 1 month ago

cphyc commented 1 month ago

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.

matthewturk commented 1 month ago

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

cphyc commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

matthewturk commented 1 month ago

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.

neutrinoceros commented 1 month ago

Fair enough. @cphyc, suit yourself !

neutrinoceros commented 1 month ago

(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 commented 1 month ago

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

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