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

Remove unused RAMSES dataset #3333

Merged
merged 1 commit into from
Sep 24, 2021
Merged

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Jun 3, 2021

This simply remove a dataset which isn't used anywhere in the code and that cannot be loaded from load_sample.

This is due to the .tar.gz having the structure:

ramses_extra_fields
├── amr_00001.out00001
├── amr_00001.out00002
├── amr_00001.out00003
├── amr_00001.out00004
├── amr_00001.out00005
├── amr_00001.out00006
├── amr_00001.out00007
├── amr_00001.out00008
├── compilation.txt
├── cooling_00001.out
├── grav_00001.out00001
├── grav_00001.out00002
├── grav_00001.out00003
├── grav_00001.out00004
├── grav_00001.out00005
├── grav_00001.out00006
├── grav_00001.out00007
├── grav_00001.out00008
├── header_00001.txt
├── hydro_00001.out00001
├── hydro_00001.out00002
├── hydro_00001.out00003
├── hydro_00001.out00004
├── hydro_00001.out00005
├── hydro_00001.out00006
├── hydro_00001.out00007
├── hydro_00001.out00008
├── hydro_file_descriptor.txt
├── info_00001.txt
├── makefile.txt
├── namelist.txt
├── part_00001.out00001
├── part_00001.out00002
├── part_00001.out00003
├── part_00001.out00004
├── part_00001.out00005
├── part_00001.out00006
├── part_00001.out00007
└── part_00001.out00008

Valid RAMSES datasets should always be contained in a folder named output_XXXXX where XXXXX should match the value in the info_XXXXX.txt filename. In this case, the structure should be ramses_extra_fields/output_00001/ for it to be loaded properly.

@neutrinoceros
Copy link
Member

I agree there is no motivation to fix this if it isn't used anywhere, and I'd rather remove it completely.
Beware that you'd need to open a complementary PR on the website repo to remove registration there too !

@matthewturk
Copy link
Member

matthewturk commented Jun 3, 2021 via email

@cphyc
Copy link
Member Author

cphyc commented Jun 3, 2021

@matthewturk no, not really. We already have ramses_extra_fields_small which does the same job but is much smaller.
@neutrinoceros this is done in yt-project/website#102!

@Xarthisius
Copy link
Member

Xarthisius commented Jun 3, 2021

Just as comment, not a review of this PR, but I don't like the fact that load_sample suddenly dictates how we structure CI or what datasets we support. Both, all previous versions of AMRVAC dataset, and this Ramses dataset are perfectly fine and can be loaded into yt. While load_sample is nice for new users, there's no need for it to be able to load ALL datasets from yt-project.org... Should I knew load_sample will end up being so disruptive I'd oppose adding it in the first place.

@neutrinoceros
Copy link
Member

I'm sorry this is causing you pain @Xarthisius. I'm sincerely hoping yt-project/website#103 will be the last of its kind, and it's intended as such.

The original version of load_sample wasn't thoroughly tested, but was merged regardless... I'm really trying to cause as little disruption as possible while still making it robust enough for shipping. I'm really sorry so much of it is falling onto you.

@cphyc
Copy link
Member Author

cphyc commented Jun 3, 2021

@Xarthisius I'm just commenting for the content of this very specific PR, not on the more global comment regarding the relation with load_sample.

The dataset I am removing cannot be loaded as is by either yt.load or yt.load_sample due to the reason I detailed in the initial comment. This could be easily fixed (simply move all stuff into a folder named output_00001 in the tarball), but since we're not using it anywhere I thought I would delete it.

@Xarthisius Xarthisius merged commit ada884d into yt-project:main Sep 24, 2021
@cphyc cphyc deleted the remove-unused-dataset branch October 19, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants