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: bring back support to group folder structure in RAMSES #3811

Merged
merged 11 commits into from
Apr 4, 2022

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Feb 15, 2022

PR Summary

This brings back support to RAMSES folder structures like output_XXXXX/group_YYYYY. Fixes #3785.
In particular, the output number should be read off the XXXXX part, not the YYYYY part.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.

Note: I have tested the bugfix on a (massive) simulation with the following script. Unfortunately, we do not have any simulation dataset with the group structure on yt-data so testing would imply more work that I can reasonably put

import yt

yt.mylog.setLevel(40)
eps = 0.0001
bbox = [[0.5-eps]*3, [0.5+eps]*3]

def test_me(path):
    print(f"Testing {path=}")
    ds = yt.load(path, bbox=bbox)
    ds.index

    assert len(ds.index.domains) > 0

    ds.r["density"]
    ds.r["DM", "particle_identity"]

test_me("/data84/obelisk/RHD/OUTPUT_DIR/output_00075")
test_me("/data84/obelisk/RHD/OUTPUT_DIR/output_00075/group_00001/")
test_me("/data84/obelisk/RHD/OUTPUT_DIR/output_00075/group_00001/info_00075.txt")
test_me("/data84/obelisk/RHD/OUTPUTS/output_00075/info_00075.txt")
test_me("/data84/obelisk/RHD/OUTPUTS/output_00075")

This brings back support to RAMSES folder structures like
output_XXXXX/group_YYYYY.

In particular, the output number should be read off the XXXXX part,
not the YYYYY part.
@cphyc cphyc added bug code frontends Things related to specific frontends labels Feb 15, 2022
@cphyc cphyc self-assigned this Feb 15, 2022
Can now load from
- output_XXXXX
- output_XXXXX/group_YYYYY
- output_XXXXX/group_YYYYY/info_XXXXX.txt
@cphyc cphyc marked this pull request as ready for review February 15, 2022 10:39
@cphyc cphyc added this to the 4.0.3 milestone Feb 15, 2022
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 think it'd be worth trying to simplify this, especially if we can't afford to test it thoroughly.

yt/frontends/ramses/data_structures.py Outdated Show resolved Hide resolved
yt/frontends/ramses/data_structures.py Outdated Show resolved Hide resolved
Co-authored-by: Clément Robert <cr52@protonmail.com>
yt/frontends/ramses/data_structures.py Outdated Show resolved Hide resolved
yt/frontends/ramses/data_structures.py Outdated Show resolved Hide resolved
@neutrinoceros
Copy link
Member

I feel much more confortable with the simplified implementation, thanks ! would it be doable to add a small unit test for the RAMSESFileSanitizer class that wouldn't require pushing another dataset to the database ?

Comment on lines +15 to +17
def generate_paths(create):
with tempfile.TemporaryDirectory() as tmpdir:
output_dir = Path(tmpdir) / "output_00123"
Copy link
Member

Choose a reason for hiding this comment

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

why not use the builtin pytest fixture tmp_path here ?

Comment on lines 102 to 103
expected_error_message = "No such file or directory '/this/does/not/exist'"
sanitizer = RAMSESFileSanitizer("/this/does/not/exist")
Copy link
Member

Choose a reason for hiding this comment

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

I anticipate this should fail on windows

Comment on lines +76 to +77
def test_invalid_sanitizing(valid_path_tuples, invalid_path_tuples):
for path in chain(*(pt.paths_to_try for pt in invalid_path_tuples)):
Copy link
Member

Choose a reason for hiding this comment

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

There's a more "pytest-ic" way to write such tests with parametrised fixtures. Here's an example: https://github.com/neutrinoceros/yt_idefix/blob/14052de1a2d1a6404fcc7c4164a4aa4d1ba283a0/tests/conftest.py#L147

I can help refactoring this particular case if you want

Copy link
Member

Choose a reason for hiding this comment

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

@cphyc I'll take that back. I'm convinced these tests could be written in a simpler form but I haven't been able to find how. I'd be prone to accept this PR as is (provided tests actually pass on windows)

@neutrinoceros
Copy link
Member

@cphyc I see that your tests are also failing on Jenkins because you forgot to ignore them on nose :)

@neutrinoceros
Copy link
Member

pre-commit fails because of an incompatibility between black and click that happened this week (psf/black#2964)
it'll be fixed on yt's main branch tomorrow with the auto-update bot. In the mean time I'm patching it locally.

@neutrinoceros
Copy link
Member

Also got an instability from matplotlib + pillow. CI should be stabilised globally by #3882

neutrinoceros
neutrinoceros previously approved these changes Apr 4, 2022
@neutrinoceros neutrinoceros merged commit a1bce43 into main Apr 4, 2022
@lumberbot-app
Copy link

lumberbot-app bot commented Apr 4, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout yt-4.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 a1bce43eeb6ad6d3dfe15db0acf2971c2053a93a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3811: BUG: bring back support to group folder structure in RAMSES'
  1. Push to a named branch:
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3811-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3811 on branch yt-4.0.x (BUG: bring back support to group folder structure in RAMSES)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@neutrinoceros neutrinoceros deleted the cphyc/issue3785 branch April 4, 2022 21:54
neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Apr 5, 2022
BUG: bring back support to group folder structure in RAMSES
(cherry picked from commit a1bce43)
@neutrinoceros
Copy link
Member

manually backported as #3886

matthewturk added a commit that referenced this pull request Apr 13, 2022
Manual backport #3811 to yt-4.0.x (BUG: bring back support to group folder structure in RAMSES)
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.

RAMSES loader does not recognize "groups" file structure
2 participants