search

yt-project / yt

Main yt repository
http://yt-project.org
Other
465 stars 276 forks source link

ENH: fix compatibility issues with newest versions of SWIFT #4921

Closed smsutherland closed 3 months ago

smsutherland commented 4 months ago

PR Summary

The SWIFT frontend is very out of date. The last significant commits are many years old, and in that time SWIFT has undergone breaking changes. This PR aims to update some of the breaks. This is by no means an exhaustive fix. It only addresses the issues I have encountered during my own use of yt with SWIFT. This includes loading SWIFT snapshots, and some issues regarding field naming.

PR Checklist

welcome[bot] commented 4 months ago

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

neutrinoceros commented 4 months ago

Thanks for working on this ! Just a very broad question for now, but are you sure this isn't an exhaustive fix, or just strongly suspecting ?

chrishavlin commented 4 months ago

nice! this may be answered by the full test suite... but any worry some of these changes would break backwards compatibility with old swift files?

neutrinoceros commented 4 months ago

I haven't looked in detail yet but yes, backward compatibility concerns should be a essential concern to reviewers.

smsutherland commented 4 months ago

Thanks for working on this ! Just a very broad question for now, but are you sure this isn't an exhaustive fix, or just strongly suspecting ?

I strongly suspect it is not exhaustive. I am very new to using yt. I am not familiar enough with all its aspects to know for certain one way or another. I only suspect it isn't exhaustive because it seems seems more likely than the alternative.

chrishavlin commented 4 months ago

I have not added any new tests, since they would require adding new sample data to test from. Both snapshots on https://yt-project.org/data/ labeled SWIFT are run on an outdated version of SWIFT from 2018.

Do you have any new sample data we could add? Would be great if possible, I'm happy to help get it where it needs to go so we can run tests against it.

smsutherland commented 4 months ago

I have not added any new tests, since they would require adding new sample data to test from. Both snapshots on https://yt-project.org/data/ labeled SWIFT are run on an outdated version of SWIFT from 2018.

Do you have any new sample data we could add? Would be great if possible, I'm happy to help get it where it needs to go so we can run tests against it.

The only SWIFT snapshots I have are many gigabytes. I can contribute one if you'd like (it's better than nothing I suppose), but a file of that size may be impractical for testing purposes or as a sample. For reference, the EAGLE_6 sample data already present is a (6.25Mpc)^3 volume and 71Mb. The snapshot I was basing my changes on is (37Mpc)^3 and 6.6Gb.

neutrinoceros commented 4 months ago

watch out, existing tests are failing. The good news is that the test suite will apparently not let us get away with backward incompatible changes after all

chrishavlin commented 4 months ago

The only SWIFT snapshots I have are many gigabytes.

Ya, that is a common issue with keeping yt's test data up to date. Given that the changes here are pretty minimal, I'd say not worth adding a file that large for the changes here.

smsutherland commented 3 months ago

And thank you for your help with this!

chrishavlin commented 3 months ago

@yt-fido test this please

chrishavlin commented 3 months ago

re-triggered the tests because the last run skipped 333 tests due to not finding files... if this is due to yt-project.org being down, those same tests will be skipped again and we should re-trigger after yt-project is back up.

EDIT: ignore this... i was looking at the wrong test, jenkins test suite is running fine.

chrishavlin commented 3 months ago

If nobody complains about it I'll merge it in a week or two.

Even with the 2 approvals now, I think it'd still be good to wait a bit to see if anyone with more expertise in swift wants to take a look.

neutrinoceros commented 3 months ago

Fine by me. I'll post-pone my personal reminder to merge this in 2 weeks from now.

welcome[bot] commented 3 months ago

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! :fireworks: