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

Add template args to DataBox #47

Merged
merged 9 commits into from
Jan 22, 2024
Merged

Add template args to DataBox #47

merged 9 commits into from
Jan 22, 2024

Conversation

brryan
Copy link
Collaborator

@brryan brryan commented Dec 8, 2023

We started getting compiler errors from DataBox due to lack of template argument. I added <Real> to the neutrino and photon DataBoxes used by the mean opacity functions.

@brryan brryan requested a review from Yurlungur December 8, 2023 19:40
@brryan
Copy link
Collaborator Author

brryan commented Dec 8, 2023

I'm not sure what's going on with the CI here E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/c/curl/libcurl4-openssl-dev_7.81.0-1ubuntu1.14_amd64.deb 404 Not Found [IP: 40.81.13.82 80] @Yurlungur @mauneyc-LANL

@Yurlungur
Copy link
Collaborator

I'm not sure what's going on with the CI here E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/c/curl/libcurl4-openssl-dev_7.81.0-1ubuntu1.14_amd64.deb 404 Not Found [IP: 40.81.13.82 80] @Yurlungur @mauneyc-LANL

I bet the VM changed. Let me see if I can fix it

@Yurlungur
Copy link
Collaborator

@brryan I fixed the CI--needed to update the workflows clone call. Some bits have rotted with the tests, though, in particular with ports-of-call.

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.

LGTM

@brryan
Copy link
Collaborator Author

brryan commented Dec 8, 2023

Thanks @Yurlungur! Can I go ahead and merge even with the ports-of-call issue in CI, or are you still looking at that?

@Yurlungur
Copy link
Collaborator

I haven't figure it out. Something screwy with the cmake. I may need @mauneyc-LANL 's help

@cmauney
Copy link

cmauney commented Dec 8, 2023

Can you clarify is this spiner or ports-of-call?

@cmauney
Copy link

cmauney commented Dec 8, 2023

Looking over cmake/SetupDeps.cmake, I don't see the spiner target added to singularity-opaq::flags, and the tests don't bring in spiner::spiner, which is probably why the tests are giving the include errors.

@Yurlungur
Copy link
Collaborator

Can you clarify is this spiner or ports-of-call?

There were a number of issues. With an update to spiner, singularity-opac needed a copy of ports-of-call again. But also HDF5 is being a problem. Whether or not spiner should use it is not being appropriately communicated between singularity-opac and spiner.

@Yurlungur
Copy link
Collaborator

OK I think I fixed it. @brryan @cmauney @mauneyc-LANL please take a look.

Copy link
Collaborator Author

@brryan brryan left a comment

Choose a reason for hiding this comment

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

LGTM

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -64,9 +66,22 @@ if (HDF5_FOUND)
else()
message("MPI::MPI_CXX provided by parent package")
endif()
else()
message(status "HDF5 is not parallel")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

message(STATUS?

Copy link

Choose a reason for hiding this comment

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

case-senstivity is weird in CMake but I think in this case it's insensitive

Copy link
Collaborator Author

@brryan brryan Dec 8, 2023

Choose a reason for hiding this comment

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

I briefly googled this out of curiosity and the first example was message(status of all things and apparently it actually does matter in this case: https://stackoverflow.com/a/35034125

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like that stack overflow is about the case of the thing the message is about, not the STATUS flag? The rest of the code is using lower-case, which is why I kept doing it

@@ -186,8 +186,8 @@ class MeanOpacity {
PORTABLE_INLINE_FUNCTION Real fromLog_(const Real lx) const {
return std::pow(10., lx);
}
Spiner::DataBox lkappaPlanck_;
Spiner::DataBox lkappaRosseland_;
Spiner::DataBox<Real> lkappaPlanck_;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for fixing the mean_s_opacity functions as well -- should we move these original changes to the using Databox = pattern you have elsewhere for consistency? I can do this if you want to hand the PR back to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that sounds good. Thanks!

@cmauney
Copy link

cmauney commented Dec 8, 2023

I still haven't wrapped my head around the original issue, but I don't see any problems with changes.If it works g2g

@Yurlungur
Copy link
Collaborator

@brryan if you wanna change the mean_opacity to the using pattern, go for it. Or just merge at will, either way is good with me. 👍

@brryan brryan merged commit b747492 into main Jan 22, 2024
1 check passed
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.

4 participants