-
Notifications
You must be signed in to change notification settings - Fork 366
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
Added Catalyst2 Build Support #3444
Added Catalyst2 Build Support #3444
Conversation
Thank you @andrewcombs! @ax3l Andrew is an intern I have been advising here at Kitware. He has been implementing Catalyst 2 functionality here in AMReX and downstream in WarpX. We have a few to dos left before it will be ready to merge. Is there anything missing we should add to the to do list? Thanks! |
This is great, thank you for the heads-up and great work! As one detail, do you think you can add the generalized particle container support as in #3349 already? (Same need to go into #3350 still). We are about to switch to that data format in WarpX and in situ vis are the last components that need migrating: |
6d765f7
to
e75c841
Compare
e75c841
to
5dfb040
Compare
9774909
to
ff9c95f
Compare
ff9c95f
to
13786a1
Compare
@andrewcombs For the TODOs, do you plan to do it in this PR or another PR? |
Likely in this PR, within the next week if time allows |
93de23b
to
8b4be4e
Compare
ca181e6
to
8ed200b
Compare
8ed200b
to
38b43e0
Compare
f5bdae7
to
b5b913a
Compare
|
0b5093a
to
5f1c449
Compare
c51c841
to
bb8c246
Compare
- Fixed Make.catalyst include directories - Added SoA particle support to Conduit blueprints - Added AMReX_Conduit_Blueprint_ParticlesI.H to Conduit Make.package - Fixed whitespaces
bb8c246
to
5aafdd3
Compare
@c-wetterer-nelson @andrewcombs quick question: this PR seems to only link against Catalyst, but add no helpers to AMReX. We can certainly merge it, but I am surprised there are not helpers (e.g., as in Ascent) being added here. Otherwise the pure find package logic for Catalyst can also be done downstream (in WarpX), no? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see now. You essentially pick up the logic from Conduit? Good for me.
Cycling CI one last time to make sure it still passes. |
- uses: actions/checkout@v3 | ||
- name: Configure | ||
run: | | ||
cmake -S . -B build \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c-wetterer-nelson I think your docker container might have a small bug. It sets an env variable catalyst_DIR
, but I think it needs to be case sensitive:
cmake -S . -B build \ | |
export Catalyst_DIR=${catalyst_DIR} | |
cmake -S . -B build \ |
https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure
CMake Error at Tools/CMake/AMReXThirdPartyLibraries.cmake:97 (find_package):
Could not find a package configuration file provided by "Catalyst" with any
of the following names:
CatalystConfig.cmake
catalyst-config.cmake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An env hint for Conduit_DIR
also needs to be set :)
Not `catalyst_DIR` (case sensitive)
Note the case sensitive spelling
Hi @ax3l I'm just getting to this now. Let me take a look at the docker image and make sure it's all working. You're right that this does not add a lot of content outside of the compile logic, and picks up the Conduit logic thanks to Catalyst's closeness to Ascent. We also have the PR in WarpX that relies on this PR. https://github.com/ECP-WarpX/WarpX/pull/4121/files |
Thanks @c-wetterer-nelson, Yes, let's ensure this PR compiles and then we take on the WarpX one :) Can you see the CI output here and pls update the docker container as needed? :) |
## Summary This PR completes the work of @andrewcombs and @c-wetterer-nelson from #3444 by adding a docker image that includes ParaView and can be used for testing. It also adds a minor fix a02c1a5 discovered when testing these changes with the current master of WarpX. ## Additional background Supersedes and closes #3444 ## Checklist The proposed changes: - [x] fix a bug or incorrect behavior in AMReX - [x] add new capabilities to AMReX - [ ] changes answers in the test suite to more than roundoff level - [ ] are likely to significantly affect the results of downstream AMReX users - [ ] include documentation in the code and/or rst files, if appropriate --------- Co-authored-by: Andrew Combs <[email protected]> Co-authored-by: Corey Wetterer-Nelson <[email protected]>
Summary
This is an equivalent implementation of Catalyst 2 to that of Ascent. It adds it to the build system so that it can be exposed to downstream codebases.
Additional background
Catalyst 2 is adding support for AMR-based datasets. Part of that initiative is full integration of the Catalyst 2 in situ analysis and visualization platform into AMReX. A pull request to WarpX is live demonstrating that implementation.
Checklists
TODO:
The proposed changes: