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] Fix issues #2866 and #4787 (incorrect code_length for RAMSES datasets) #4934

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lenoble-lab
Copy link
Contributor

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 chrishavlin added bug code frontends Things related to specific frontends labels Jun 26, 2024
@chrishavlin
Copy link
Contributor

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 cphyc changed the title correct issue ? [BUG] Fix issues #2866 and #4787 (incorrect code_length for RAMSES datasets) Jun 27, 2024
@cphyc
Copy link
Member

cphyc commented Jun 27, 2024

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 13dc4fb (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 :)

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.

3 participants