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

ENH: Adding Cholla Frontend #3663

Merged
merged 84 commits into from
Sep 22, 2022
Merged

ENH: Adding Cholla Frontend #3663

merged 84 commits into from
Sep 22, 2022

Conversation

chummels
Copy link
Member

@chummels chummels commented Nov 11, 2021

This adds a frontend to yt for working with datasets from the CHOLLA code (https://github.com/cholla-hydro/cholla). It replaces the previous attempt at this from a few years ago (PR #1944 )

CHOLLA is a super straightforward code format: it's a fixed-resolution grid code saved to HDF5 files with attributes storing simulation parameters. For simplicity, I've also made it so nprocs=1, so it reads a single grid for the whole dataset, since it's unigrid. After I get this working, I'll make yt chunk it into smaller-sized chunks for for efficiency. But for now, I just want something that works.

The frontend works so far for basic things like reporting fields and parameters and such. But when one attempts to actually make a slice or something, it fails with a Arrays are not of correct shape error. The main remaining hurdles are getting the _read_fluid_selection() and _read_chunk_data() functions working in io.py.

Any assistance in finishing up this frontend would be greatly appreciated. The CHOLLA code author, @evaneschneider, and her students are interested in contributing to active development in yt when they can actually get their code to be read in successfully.

For testing purposes, a 1-zone CHOLLA test dataset can be found here: http://chummels.org/cholla/1.h5 .

chummels and others added 2 commits August 25, 2022 14:57
Co-authored-by: Clément Robert <cr52@protonmail.com>
@chummels
Copy link
Member Author

I added some basic tests which just ensure that the test file is recognized as a CHOLLA dataset, that it can load it, and that it has the fields that are expected and can access them. Are there any other canonical tests that are used to test out frontends or is this enough for now?

Co-authored-by: Clément Robert <cr52@protonmail.com>
@matthewturk
Copy link
Member

Looks like this is nearly done -- are we waiting for answer tests? Is that something someone else could help out with?

@matthewturk matthewturk added this to the 4.1.0 milestone Sep 6, 2022
@matthewturk
Copy link
Member

pre-commit.ci autofix

@chummels
Copy link
Member Author

chummels commented Sep 6, 2022

Yeah, I've been slow on this front, as I'm at some conferences in Switzerland for a couple of weeks. If we want answer tests, I can potentially get to this, but it won't be until I get back. But if someone else wanted to craft a basic answer test, by all means go for it.

@neutrinoceros
Copy link
Member

If we're not able to finish this in the next couple weeks I'd suggest leaving it to yt 4.2 which - the way I envision it - should happen much closer in time than 4.1 is to 4.0

@matthewturk
Copy link
Member

@neutrinoceros I think we need to hold 4.1 until this is in, and I will work on this concurrently with finishing up #3421 . @chummels can you enumerate what is required still, and I can take it on?

@jzuhone
Copy link
Contributor

jzuhone commented Sep 7, 2022

I agree, this should go in 4.1

@chummels
Copy link
Member Author

chummels commented Sep 9, 2022

@matthewturk I think all that remains to be done for this PR is the addition of an answer test or two using the dataset that is already being used in the tests. It's a super simple dataset with a density gradient across the grids, so I guess just make a ProjectionPlot and SlicePlot and PhasePlot answer test and then call it a day? I am not sure, but that seems like it should be sufficient?

@matthewturk
Copy link
Member

I've gone ahead and added some simple tests. These work locally, but it's hard for me know if I did everything properly, so I guess we'll see what fido says.

@neutrinoceros
Copy link
Member

close/open to refresh CI now that unrelated failures are resolved on main

@neutrinoceros
Copy link
Member

It seems that the new tests break pytest. I think there is a precedent for this and you could simply ignore the new test module on pytest with this patch:

diff --git a/pyproject.toml b/pyproject.toml
index 6306dea36..9b80ade02 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -67,6 +67,7 @@ addopts = '''
     --ignore-glob='*_nose.py'
     --ignore='yt/frontends/owls_subfind/tests/test_outputs.py'
     --ignore='yt/frontends/ramses/tests/test_outputs.py'
+    --ignore='yt/frontends/cholla/tests/test_outputs.py'
 '''

@neutrinoceros
Copy link
Member

FTR I just merged from main, but I really meant to do this for #3421. This should not cause any trouble other than wasting CI time. Sorry folks

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 don't have any blocking comments left. I believe Matt should be able to merge this even as a co-author.

@matthewturk matthewturk merged commit aed236f into yt-project:main Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code frontends Things related to specific frontends new feature Something fun and new!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants