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

Added Catalyst2 Build Support #3444

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

Conversation

andrewcombs
Copy link

@andrewcombs andrewcombs commented Jul 24, 2023

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:

  • Add a github workflow test

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • 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

@c-wetterer-nelson
Copy link
Contributor

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!

@ax3l
Copy link
Member

ax3l commented Jul 25, 2023

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:
ECP-WarpX/WarpX#3850

@WeiqunZhang
Copy link
Member

@andrewcombs For the TODOs, do you plan to do it in this PR or another PR?

@andrewcombs
Copy link
Author

Likely in this PR, within the next week if time allows

@c-wetterer-nelson
Copy link
Contributor

c-wetterer-nelson commented Dec 12, 2023

whoops. looks like I built the catalyst image for arm64. fixing that now.
updated the container to amd64. hopefully that'll let the actions run. gotta rerun the new catalyst build test now.

@c-wetterer-nelson c-wetterer-nelson force-pushed the catalyst2-support branch 3 times, most recently from 0b5093a to 5f1c449 Compare December 13, 2023 21:05
@c-wetterer-nelson c-wetterer-nelson force-pushed the catalyst2-support branch 2 times, most recently from c51c841 to bb8c246 Compare December 20, 2023 22:08
- Fixed Make.catalyst include directories

- Added SoA particle support to Conduit blueprints

- Added AMReX_Conduit_Blueprint_ParticlesI.H to Conduit Make.package

- Fixed whitespaces
@ax3l
Copy link
Member

ax3l commented Mar 29, 2024

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

Copy link
Member

@ax3l ax3l left a 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.

@ax3l
Copy link
Member

ax3l commented Mar 29, 2024

Cycling CI one last time to make sure it still passes.

@ax3l ax3l closed this Mar 29, 2024
@ax3l ax3l reopened this Mar 29, 2024
@ax3l ax3l closed this Mar 29, 2024
@ax3l ax3l reopened this Mar 29, 2024
.github/workflows/catalyst.yml Outdated Show resolved Hide resolved
- uses: actions/checkout@v3
- name: Configure
run: |
cmake -S . -B build \
Copy link
Member

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:

Suggested change
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

Copy link
Member

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
.github/workflows/catalyst.yml Outdated Show resolved Hide resolved
@c-wetterer-nelson
Copy link
Contributor

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

@ax3l
Copy link
Member

ax3l commented Apr 8, 2024

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? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants