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

eospac serialization fix #417

Merged
merged 6 commits into from
Sep 13, 2024
Merged

eospac serialization fix #417

merged 6 commits into from
Sep 13, 2024

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Sep 11, 2024

PR Summary

In the previous serialization MR #410 I punted one issue with EOSPAC. This MR now fixes the inconsistencies. I test EOSPAC with shared memory in the unit tests and I update the default cmake option to assume shared memory is available.

I also had to add some link libraries (available in the standard library) that HDF5 depends on to FindHDF5 as cmake was not properly adding them.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

@Yurlungur Yurlungur self-assigned this Sep 11, 2024
@Yurlungur Yurlungur linked an issue Sep 11, 2024 that may be closed by this pull request
@Yurlungur
Copy link
Collaborator Author

I also needed to more cleanly separate the closure tests as they should be disabled when closure is off.

Copy link
Collaborator

@aematts aematts left a comment

Choose a reason for hiding this comment

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

Looks fine to my untrained eye.

Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

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

Looks good! Minor questions

Comment on lines +50 to +57
if (SINGULARITY_BUILD_CLOSURE)
add_executable(
closure_unit_tests
catch2_define.cpp
test_closure_pte.cpp
test_kpt_models.cpp
)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I added the closure unit tests, I was copying the approach for the tabulated unit tests and trying to simplify the CMake so that each option gets applied to its relevant executables one-by-one. Is it better to complicate the CMake logic to avoid compiling the test rather than rely on #ifdef statements to make sure its basically an empty test?

Put another way, in general should we be trying to push more logic into the CMake and less into the source via preprocessor flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I don't know. Either is fine. I went overboard here, the tests weren't wrapped in preprocessor protection in this case or protected in the cmake so I just did both to be safe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty ignorant to the finer points of CMake so I'm happy to go either way. But if there's a superior way, let me know. @mauneyc-LANL do you have opinions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Yeah happy to revise this structure if there's one that makes sense.

singularity-eos/eos/eos_eospac.hpp Show resolved Hide resolved
test/test_eos_tabulated.cpp Outdated Show resolved Hide resolved
@Yurlungur Yurlungur merged commit 2358725 into main Sep 13, 2024
5 checks passed
@Yurlungur Yurlungur deleted the jmm/eospac-serialization-fix branch September 13, 2024 16:20
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.

EOSPAC backend with serialization/deserialization
4 participants