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: improve boxlib data type detection #4402

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

yut23
Copy link
Member

@yut23 yut23 commented Apr 6, 2023

PR Summary

Make CastroDataset and MaestroDataset inherit from AMReXDataset so they supercede it when calling yt.load. This allows loading output files from those projects without needing to pass hint.

I don't personally use Nyx, WarpX, or Orion, so any input from those projects would be appreciated.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@yut23
Copy link
Member Author

yut23 commented Apr 6, 2023

Some of the Castro (and Maestro) sample datasets are from before we migrated from BoxLib to AMReX, so the job_info files don't contain the string "amrex". That means CastroDataset and BoxlibDataset are valid candidates for yt.load, but AMReXDataset is not. This exposed a fun edge case in find_lowest_subclasses, where it would add AMReXDataset to the list of candidates.

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.

Thank you for tackling this long standing problem with the boxlib frontend !

This seems fine to me as a non-expert of this frontend (and the codes it relates too), but I'd very much like @zingale's opinion on this (or possibly recommendations for any other boxlib experts).

I think we should also clearly state how the inheritance hierarchy is designed: is it reflecting how the supported codes relate to each other ? is it a pure abstraction aiming for maximal code reuse ? is it purely historical ? anything else ?

yt/utilities/hierarchy_inspection.py Outdated Show resolved Hide resolved
@neutrinoceros neutrinoceros added enhancement Making something better code frontends Things related to specific frontends labels Apr 7, 2023
@zingale
Copy link
Member

zingale commented Apr 7, 2023

I will test this today (and I can test Nyx). @ax3l can you test WarpX on this PR?

@zingale
Copy link
Member

zingale commented Apr 7, 2023

I've confirmed that this works with Nyx, Castro, and MAESTROeX without needing to use the hint keyword

zingale
zingale previously approved these changes Apr 8, 2023
@neutrinoceros
Copy link
Member

@yut23 can you cleanup the branch history ? It looks like the first commit is only made of changes that were incorporated already

neutrinoceros
neutrinoceros previously approved these changes Apr 15, 2023
Make `CastroDataset` and `MaestroDataset` inherit from `AMReXDataset` so
they supercede it when calling yt.load.

Also remove a useless `__init__` override from `AMReXDataset`.
@neutrinoceros
Copy link
Member

I actually think the change is small enough that we don't need too much extra manual validation. I'll give @ax3l an other week or so to tune in if he wants but otherwise I think I'll just merge this.

@neutrinoceros neutrinoceros merged commit eb57d39 into yt-project:main Apr 25, 2023
10 checks passed
@yut23 yut23 deleted the improve_boxlib_detection branch May 18, 2023 15:35
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 enhancement Making something better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants