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: allow boxlib frontend to read 1D cylindrical datasets #4758

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

yut23
Copy link
Member

@yut23 yut23 commented Dec 12, 2023

PR Summary

Removes the check that disallows cylindrical coordinates in 1D. It looks like yt learned how to support these sometime since this code was added in 2014. It also fixes an error in _parse_header_file() introduced by #4641.

I tested against an example dataset output by https://github.com/AMReX-Astro/Castro/tree/main/Exec/hydro_tests/Sedov using inputs.1d.cyl, and the cell volumes checked out (proportional to 1/r).

PR Checklist

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

@neutrinoceros neutrinoceros added code frontends Things related to specific frontends bug labels Dec 12, 2023
@neutrinoceros
Copy link
Member

I'm triaging this as a bug fix for now because it removes a blocking problem to anyone who would need the functionality, and the change is currently very small, but let's re-assess once we see test go green again.

@neutrinoceros
Copy link
Member

Actually, failures are clearly unrelated.

@neutrinoceros neutrinoceros added this to the 4.3.1 milestone Dec 12, 2023
@neutrinoceros
Copy link
Member

For what it's worth I've issued a fix for those failures at #4759

neutrinoceros
neutrinoceros previously approved these changes Dec 12, 2023
@neutrinoceros
Copy link
Member

@yut23 can you rebase to current main so we can get CI green ?

@yut23
Copy link
Member Author

yut23 commented Dec 12, 2023

It might make more sense to split out the _parse_header_file() fix into a separate PR, since that's an actual bug fix, while the rest is an enhancement.

@neutrinoceros
Copy link
Member

Works for me, but I think this is also fine as is.

@yut23
Copy link
Member Author

yut23 commented Dec 12, 2023

We should probably add a sample data set to test with. I'm not sure what the process for that is, though.

@neutrinoceros
Copy link
Member

You're right. And storage basically free for a 1D dataset so let's do it.
You can use yt upload on the command line to share the data sample. Then open a PR to the yt-project/website repo (see previously merged ones for an example). You will also need to update the data sample yaml registry in this repo (you may do it within this PR).

@cphyc
Copy link
Member

cphyc commented Jan 12, 2024

@yt-fido test this please.

cphyc
cphyc previously approved these changes Jan 12, 2024
Copy link
Member

@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.

This looks good to me :)

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.

there appears to be a problem with the new data sample:

import yt

ds = yt.load_sample("castro_sedov_1d_cyl_plt00150")
ds = yt.load_sample("castro_sedov_1d_cyl_plt00150")

fails with FileNotFoundError (the exception is raised on the first call if data is already installed, and on the second otherwise). I'm looking into it.

@neutrinoceros neutrinoceros merged commit d1903de into yt-project:main Jan 12, 2024
13 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Jan 12, 2024
@yut23 yut23 deleted the fix-boxlib-1d-cyl branch February 8, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code frontends Things related to specific frontends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants