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

rockstar+RAMSES functionality #238

Conversation

AnatoleStorck
Copy link
Contributor

Added functionality for adding RAMSES datasets with rockstar-galaxies catalogues using yt to tangos.

@apontzen
Copy link
Member

apontzen commented Nov 2, 2023

Thanks @AnatoleStorck @cphyc - do you understand why the enzo test database now fails to build?

If needed you can try this locally by running export INTEGRATION_TESTING=1; ./build.sh in the test_tutorial_build folder

@AnatoleStorck
Copy link
Contributor Author

I have moved the new functions in the general YT handler which were causing the Enzo test to fail (I still do not understand why) into the input handler for RAMSES+rockstar. The Enzo test now passes. Since this pull request is for supporting RAMSES+rockstar functionality, this should be fine.

@apontzen
Copy link
Member

To me it looks like people who have developed their own input handlers that derive from YtInputHandler will be tripped up by this. Unfortunately I think it's necessary to solve the underlying problem rather than remove functionality from the crucial base class.

(I am not sure why the tests are not actually running on Github Actions, by the way - will see if I can figure that out)

@AnatoleStorck AnatoleStorck force-pushed the AnatoleStorck-rockstarRAMSESupdate branch from 9398589 to 3e51429 Compare November 10, 2023 14:52
@AnatoleStorck
Copy link
Contributor Author

AnatoleStorck commented Nov 10, 2023

@cphyc and I fixed the issue, occurring due to the handler not importing the statfile if it exists.

Copy link
Contributor

@cphyc cphyc left a comment

Choose a reason for hiding this comment

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

Looks great to me! I've left extensive comments to make it more yt-esque!

tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
tangos/tools/add_simulation.py Outdated Show resolved Hide resolved
tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
tangos/input_handlers/yt.py Show resolved Hide resolved
Comment on lines +241 to +247
overdir = self._extension_to_filename("")
snapfiles = glob.glob(overdir+ts_extension[:2]+len(ts_extension[2:].split('/')[0])*'?')
rockfiles = glob.glob(overdir+"out_*.list")
sortind = np.array([int(rname.split('.')[0].split('_')[-1]) for rname in rockfiles])
sortord = np.argsort(sortind)
snapfiles.sort()
rockfiles = np.array(rockfiles)[sortord]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know where this piece of code comes from? It's really hard to make sense out of it and I'm wondering if we could simplify it somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the YtRamsesRockstarInputHandler was first created by taking the YtEnzoRockstarInputHandler and adapting it to ramses datasets. This function is taken from there.

tangos/input_handlers/yt.py Outdated Show resolved Hide resolved
@apontzen
Copy link
Member

If you can pull master into this branch, it will also run the unit tests. At the moment only the integration test is running due to a configuration error in github actions.

Copy link
Contributor Author

@AnatoleStorck AnatoleStorck left a comment

Choose a reason for hiding this comment

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

Thank you for the suggestions! I will also remove the logging changes

Comment on lines +241 to +247
overdir = self._extension_to_filename("")
snapfiles = glob.glob(overdir+ts_extension[:2]+len(ts_extension[2:].split('/')[0])*'?')
rockfiles = glob.glob(overdir+"out_*.list")
sortind = np.array([int(rname.split('.')[0].split('_')[-1]) for rname in rockfiles])
sortord = np.argsort(sortind)
snapfiles.sort()
rockfiles = np.array(rockfiles)[sortord]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the YtRamsesRockstarInputHandler was first created by taking the YtEnzoRockstarInputHandler and adapting it to ramses datasets. This function is taken from there.

tangos/tools/add_simulation.py Outdated Show resolved Hide resolved
apontzen added a commit that referenced this pull request Sep 19, 2024
Rockstar + RAMSES functionalities [rebase of #238]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants