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

add PlotFileData bindings #320

Open
wants to merge 18 commits into
base: development
Choose a base branch
from

Conversation

BenWibking
Copy link

@BenWibking BenWibking commented May 19, 2024

Enable reading plotfiles with pyAMReX. Also adds a script in tools/ to create images from plotfiles.

Example use-case: https://github.com/BenWibking/plotfile-viewer.

Closes #317.

@ax3l ax3l requested review from ax3l and atmyers May 30, 2024 06:03
@ax3l ax3l added the enhancement New feature or request label May 30, 2024
@ax3l
Copy link
Member

ax3l commented May 30, 2024

@BenWibking thanks a lot, looking good! Ping us when you are ready with the unit test and we can include it maybe in this month's release even?

@BenWibking
Copy link
Author

BenWibking commented May 30, 2024

@ax3l I've added a unit test to read a 3D plotfile with a single level. I put the binary data in the PR, but I can move that to some more appropriate place if needed.

When I try to add a test for a 2D plotfile, I get this error:

tests/test_plotfiledata_2d.py:8: in <module>
    import amrex.space2d as amr
../myenv/lib/python3.12/site-packages/amrex/space2d/__init__.py:22: in <module>
    from . import amrex_2d_pybind
E   ImportError: generic_type: type "AMReX" is already registered!

Do 2D tests need to have some special configuration?

@BenWibking BenWibking marked this pull request as ready for review May 31, 2024 19:50
@BenWibking BenWibking changed the title [WIP] add PlotFileData bindings add PlotFileData bindings May 31, 2024
@BenWibking
Copy link
Author

@ax3l Aside from the 2D test issue, everything should be working now.

@@ -0,0 +1,22 @@
HyperCLaw-V1.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenWibking Thanks for the question!

Yes, please do not check in binary files into the repository, to keep it small. We clone pyAMReX in many workflows, e.g., superbuilds and a slick repo size is important.

Could you potentially for the test generate the data on the fly, e.g., by running a generating function first that you read back?

Copy link
Member

@ax3l ax3l Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised the pre-commit.ci check check-added-large-files that we run on CI does not fail on the 6 MB binary file... concerning :-o

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that I don't know how to independently verify that the plotfile generation was correct if it's generated on the fly. Would it be possible to upload the test files as an extra .tar.gz file in a GitHub release, or to re-use the test data on the yt website under "boxlib frontend"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downloading the test data on the fly is ok.
There is a downside to this, making some test runs slow and others impossible (w/o internet).

My concern is that I don't know how to independently verify that the plotfile generation was correct if it's generated on the fly.

I think this can be verified once manually or in a separate test. (We also do not verify a downloaded files continuously, unless I misunderstand something here.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. I think verifying the checksum for the generated plotfile would work instead of downloading a file (since that should be checksummed anyway).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenWibking when you have the time, can you still update this? I would merge the PR once the large files are not checked in anymore :)

tools/plot_plotfile_2d.py Show resolved Hide resolved
tools/plot_plotfile_2d.py Outdated Show resolved Hide resolved
tools/plot_plotfile_2d.py Show resolved Hide resolved
tests/test_plotfiledata.py Show resolved Hide resolved
tests/test_plotfiledata.py Show resolved Hide resolved
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
@ax3l ax3l self-assigned this Jun 12, 2024

@pytest.mark.skipif(amr.Config.spacedim != 2, reason="Requires AMREX_SPACEDIM = 2")
def plot_plotfile_2d(filename, level=0):
import amrex.space2d as amr

Check notice

Code scanning / CodeQL

Module is imported more than once Note

This import of module amrex.space2d is redundant, as it was previously imported
on line 51
.

@pytest.mark.skipif(amr.Config.spacedim != 3, reason="Requires AMREX_SPACEDIM = 3")
def test_plotfiledata_read():
import amrex.space3d as amr

Check notice

Code scanning / CodeQL

Module is imported more than once Note

This import of module amrex.space3d is redundant, as it was previously imported
on line 8
.
@baperry2
Copy link

This is awesome! I was hoping for this capability. In general, to use pyamrex for pre/postprocessing workflows having as much I/O capability as possible is super helpful.

Another example would be reading/writing individual FAB data (appears to be commented out here:

), which would have been useful for something I was looking at doing a while ago, but am not longer planning on.

@BenWibking
Copy link
Author

This is awesome! I was hoping for this capability. In general, to use pyamrex for pre/postprocessing workflows having as much I/O capability as possible is super helpful.

Another example would be reading/writing individual FAB data (appears to be commented out here:

), which would have been useful for something I was looking at doing a while ago, but am not longer planning on.

That would be useful. I'm not sure why that's commented out. Maybe @ax3l knows?

In the future, I would like to add the ability to lazy-load FABs for out-of-core analysis, but that might require additional changes in AMReX.

@ax3l
Copy link
Member

ax3l commented Jun 18, 2024

Awesome, yes those function should be added, too, but maybe in a separate PR.

There are some comments in #82 and tests/test_farraybox.py how to bind C++ iostreams and Python io. I simply had no time to test it and no need yet, but PRs are very welcome on it (with a test please) 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bindings for PlotFileData
3 participants