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

Update submodules, API changes, tracer swarm packs, Monte Carlo packs, formatting CI #225

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

AstroBarker
Copy link
Collaborator

@AstroBarker AstroBarker commented Jan 6, 2025

PR Summary

Far overdue, this PR updates parthenon, singularity-eos, and singularity-opac and makes necessary changes to work with their updated versions. There are a number of additional changes that impact performance.

  • Updates to MHD (API changes for packing)

  • Overhauls to swarm API

  • The advection problem's tracers were refactored due to a bug on multiple blocks

  • Some updates to BCs, necessitated by parthenon changes. Probably more simplification can be done.

    • Need to use gr_outflow instead of outflow as the latter is now a Parthenon builtin that isn't spacetime / geometry aware.
  • The fast logs of singularity-eos are default disabled.

  • Updates neutrino mean opacity API

  • Tracers:

    • Use SwarmPacks
    • Refactored to work on MeshData instead of MeshBlockData
    • tracer swarm variables are SWARM_VARIABLE now, so access with e.g., tv::rho(), tv::rho::name() e.g., (for namespace tv = tracer_variables; to enable type based packing.
  • Started porting moments code to packs

  • Monte carlo code was ported to swarm and sparse packs. Soon: to MeshData

  • Monte Carlo swarm variables are VARIABLE, SWARM_VARIABLE or TENSOR_VARIABLE.

  • MOCMC variables are SWARM_VARIABLE and TENSOR_SWARM.

  • A consequence of the packing and template variable names is that the neutrino species is now compile time. It defaults to all 3 flavors enabled and is controlled as -DPHOEBUS_DO_NU_ELECTRON=On, -DPHOEBUS_DO_NU_ELECTRON_ANTI=On, and -DPHOEBUS_DO_NU_HEAVY=On

  • I have implemented MeshData level swarm comms routines (from Jaybenne)

  • I have implemented a temporary MeshData level Monte Carlo MomentFluidSource routine that for loops over blocks.

    • Later this should be implemented to other swarms machinery
  • Also, the github ubuntu-latest changed and clang-format-12 was no longer available. I have moved us to installing clang-format with pip, pinned at version 12.0.1. Which was not compatible with ubuntu's v12...

  • Weirdness seems to have crept in to the regression testing. Some tests, despite producing an identical gold file (when ran with --upgold) failed tests due to how hdf5 was loaded. Unsure what caused this change, but a fix was implemented.

@AstroBarker AstroBarker added enhancement New feature or request build build system refactor labels Jan 6, 2025
@AstroBarker AstroBarker requested a review from Yurlungur January 6, 2025 18:09
@AstroBarker AstroBarker changed the title Update submodules, API changes, tracer swarm packs Update submodules, API changes, tracer swarm packs, formatting CI Jan 6, 2025
@AstroBarker AstroBarker changed the title Update submodules, API changes, tracer swarm packs, formatting CI WIP: Update submodules, API changes, tracer swarm packs, formatting CI Jan 9, 2025
@AstroBarker AstroBarker changed the title WIP: Update submodules, API changes, tracer swarm packs, formatting CI Update submodules, API changes, tracer swarm packs, Monte Carlo packs, formatting CI Jan 30, 2025
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

This was a BIG effort. Thanks @AstroBarker . Some minor nitpicks that should be addressed before merging. But I'm approving now. Nice work!

cmake/Transport.cmake Outdated Show resolved Hide resolved
inputs/blast_wave.pin Outdated Show resolved Hide resolved
inputs/homogeneous_sphere.pin Outdated Show resolved Hide resolved
src/compile_constants.hpp.in Outdated Show resolved Hide resolved
src/phoebus_boundaries/phoebus_boundaries.cpp Outdated Show resolved Hide resolved
src/phoebus_driver.cpp Outdated Show resolved Hide resolved
src/phoebus_utils/variables.hpp Show resolved Hide resolved
tst/regression/regression_test.py Show resolved Hide resolved
src/radiation/geodesics.hpp Show resolved Hide resolved
src/radiation/geodesics.hpp Outdated Show resolved Hide resolved
Comment on lines +720 to +721
template <>
TaskStatus ApplyFloors<MeshData<Real>>(MeshData<Real> *rc) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if the specialization isn't working properly. Maybe worth double checking that this version of the code is actually being called. (And same for the others...)

template <>
TaskStatus ApplyFloors<MeshData<Real>>(MeshData<Real> *rc) {
for (int b = 0; b < rc->NumBlocks(); b++) {
ApplyFloors(rc->GetBlockData(b).get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does GetBlockData(b) definitely return the right stage? it should I think? Might be worth double checking the name that gets spit out here

Copy link
Collaborator

@bprather bprather Feb 3, 2025

Choose a reason for hiding this comment

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

Ah! This is straight from a MeshData object -- that provides the correct stage, yeah.

pmb->meshblock_data.Get(); does not -- I was sensitive because we just got bit by this in KHARMA: parthenon-hpc-lab/parthenon#1220

Personally I like the foreach pattern Philipp suggests in that issue over numerical iteration.

@Yurlungur
Copy link
Collaborator

Since b3d4ebf isn't working lets just revert for now. Can be a fix for a later MR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason these BC flags are preserved? is this a cartesian coords thing

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.

4 participants