search

yt-project / yt

Main yt repository
http://yt-project.org
Other
465 stars 276 forks source link

[BUG] Fix issues #2866 and #4787 (incorrect code_length for RAMSES datasets) #4934

Open Lenoble-lab opened 3 months ago

Lenoble-lab commented 3 months ago

Hello, It seems there is an issue with length unit for ramses output.

The bug has already been identified in 2866 and 4787.

Here I propose a modification of two lines in data_structure.

I don't understand why the length_unit is scaled with the boxlen. To me, it should not as then, we loose the units if boxlen != 1. The correction induced is to change the yt's boxlen which should be different from 1 if we change the units.

Thanks, Romain

chrishavlin commented 3 months ago

Thanks for the PR @Lenoble-lab ! Looks OK to me but would be good to have eyes on this from someone familiar with Ramses -- @cphyc are you available for a quick look?

The failing 3.12 test is unrelated, but the failing pre-commit check will need to be fixed manually:

yt/frontends/ramses/data_structures.py:949:9: F841 Local variable `boxlen` is assigned to but never used
cphyc commented 3 months ago

I am still not 100% sure this is what we want, but I'm failing to explain why :) I am also unhappy that this significantly-breaking change didn't trigger any test failure. That means we've never tested that the units were correct for non-cosmological datasets. Especially, I just tested locally, and it seems that your fix doesn't solve 100% of the issue. For example

In [5]: ds = yt.load_sample("DICEGalaxyDisk_nonCosmological")

In [6]: ds.r["x"].to("code_length").max()   # Duh?
Out[6]: unyt_quantity(1.7578125, 'code_length')

In [7]: ds.domain_right_edge.to("kpc")
Out[7]: unyt_array([150.00000002, 150.00000002, 150.00000002], 'kpc')

In [8]: ds.r["x"].to("kpc").max()
Out[8]: unyt_quantity(1.7578125, 'kpc')

However, on 13dc4fbcba564439c90e9bdd8db013f797dfc9a (the main branch), I get the following:

In [1]: ds = yt.load_sample("DICEGalaxyDisk_nonCosmological")

In [2]: ds.r["x"].to("kpc").max()
Out[2]: unyt_quantity(149.41406252, 'kpc')

In [3]: ds.domain_right_edge.to("kpc")
Out[3]: unyt_array([150.00000002, 150.00000002, 150.00000002], 'kpc')

which seems like a better solution :)