ENH: fix compatibility issues with newest versions of SWIFT #4921
Conversation
|
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! |
|
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 ? |
|
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? |
|
I haven't looked in detail yet but yes, backward compatibility concerns should be a essential concern to reviewers. |
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. |
yt/frontends/swift/io.py
Outdated
| @@ -66,7 +66,7 @@ def _get_smoothing_length(self, sub_file, pdtype=None, pshape=None): | |||
| pcount = f["/Header"].attrs["NumPart_ThisFile"][ind].astype("int64") | |||
| pcount = np.clip(pcount - si, 0, ei - si) | |||
| # we upscale to float64 | |||
| hsml = f[ptype]["SmoothingLength"][si:ei, ...] | |||
| hsml = f[ptype]["SmoothingLengths"][si:ei, ...] | |||
There was a problem hiding this comment.
Here we'd probably need to try both keys for backward compatibility. It's okay to give priority to the newest one but it would also help to add a comment specifying (at least broadly) which version of SWIFT made the change.
| @@ -15,7 +16,7 @@ class SwiftParticleFile(ParticleFile): | |||
| class SwiftDataset(SPHDataset): | |||
| _load_requirements = ["h5py"] | |||
| _index_class = SPHParticleIndex | |||
| _field_info_class = SPHFieldInfo | |||
| _field_info_class = SwiftFieldInfo | |||
There was a problem hiding this comment.
not clear to me what are the implications of this change in terms of backward compatibility
There was a problem hiding this comment.
Anyone want to comment here ? For the record, I don't think it's critical enough to hold back the PR, but I'd like an answer if possible.
There was a problem hiding this comment.
Someone with more of an expertise in swift data might want to chime in, but I just spent a while walking through the initialization and aliasing that happens and I don't see any issues. It will still check for all the fields defined by SPHFieldInfo, and the additional aliasing that SwiftFieldInfo adds will only happen if the new fields exist within the dataset.
| @@ -113,7 +113,7 @@ def _parse_parameter_file(self): | |||
| self.dimensionality = int(header["Dimension"]) | |||
|
|
|||
| # SWIFT is either all periodic, or not periodic at all | |||
| periodic = int(runtime_parameters["PeriodicBoundariesOn"]) | |||
| periodic = int(parameters["InitialConditions:periodic"]) | |||
There was a problem hiding this comment.
Here too, we should really be checking both for new and old names.
| self.omega_matter = float(parameters["Cosmology:Omega_b"]) + float( | ||
| parameters["Cosmology:Omega_cdm"] | ||
| ) |
There was a problem hiding this comment.
Is this a bug fix or is the Cosmology:Omega_cdm key new in some version of SWIFT ? If old versions don't have it, we need to fallback to the old value somehow.
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. |
|
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 |
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. |
| @@ -15,7 +16,7 @@ class SwiftParticleFile(ParticleFile): | |||
| class SwiftDataset(SPHDataset): | |||
| _load_requirements = ["h5py"] | |||
| _index_class = SPHParticleIndex | |||
| _field_info_class = SPHFieldInfo | |||
| _field_info_class = SwiftFieldInfo | |||
There was a problem hiding this comment.
Anyone want to comment here ? For the record, I don't think it's critical enough to hold back the PR, but I'd like an answer if possible.
In newer versions of SWIFT, Cosmology:Omega_m still exists, but it just has a value of -1. We need to check whether Omega_cdm exists, not whether Omega_m exists.
|
And thank you for your help with this! |
|
@yt-fido test this please |
|
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. |
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. |
|
Fine by me. I'll post-pone my personal reminder to merge this in 2 weeks from now. |
|
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆 |
neutrinoceros
changed the title
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
New features are documented, with docstrings and narrative docs
No new features have been added.
Adds a test for any bugs fixed. Adds tests for new features.
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.