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 #124

Merged
merged 8 commits into from
Aug 23, 2022
Merged

update submodules #124

merged 8 commits into from
Aug 23, 2022

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Aug 16, 2022

PR Summary

PR Checklist

This PR updates us to the newest parthenon. I plan to use this to check against Philipp Grete's I/O fix. But it also brings us up to date with all of @lroberts36 sparse fixes, and moves some more things into "in-one," which maybe will help with performance.

One question to ask is if we wait to merge this in until Philipp's IO change is in main or just do two PRs.

I'm also working on updating singularity-eos so we can be up to date there. This requires a few minor changes to the build system. One annoying aspect is I found a build system bug in singularity-eos which I'm fixing at the moment.

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by calling scripts/bash/format.sh.
  • Explain what you did.

@Yurlungur
Copy link
Collaborator Author

Fix for singularity-eos build system now in place. But I need the assert fix in parthenon to be merged in before our tests will pass:
parthenon-hpc-lab/parthenon#716

@Yurlungur
Copy link
Collaborator Author

Ok I think everything should work now, modulo Philipp's I/O thing. Should we merge this now (when tests pass) or wait for that to get into Parthenon?

@Yurlungur
Copy link
Collaborator Author

Oops forgot to push my cmake fixes

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

This LGTM except for the one significant comment below. Once my question there is answered will approve.

src/pgen/radiation_advection.cpp Show resolved Hide resolved
src/pgen/radiation_equilibration.cpp Show resolved Hide resolved
src/phoebus_driver.cpp Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator Author

Chicoma now works again. See parthenon-hpc-lab/parthenon#720

@Yurlungur
Copy link
Collaborator Author

@lroberts36 are you happy with this? If so I think it's ready for merge. I think we should update to the new HDF5 format in a separate PR, as this one ended up being more than a few headaches, and I'd like those resolved with a PR/commit number before introducing more changes.

@lroberts36 lroberts36 self-requested a review August 23, 2022 00:06
Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM

@Yurlungur Yurlungur merged commit bc6a543 into main Aug 23, 2022
@Yurlungur Yurlungur deleted the jmm/update-submodules branch August 23, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants