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

Geant4 v11 fix #907

Merged
merged 29 commits into from
Dec 7, 2023
Merged

Conversation

ahnaf-tahmid-chowdhury
Copy link
Member

I created a backup branch and reverted all the changes on my develop branch in the DAGMC repository. It seems like my previous PR got deleted in the process. Thus, I have created a new one. I apologize for any inconvenience this may have caused.

@ahnaf-tahmid-chowdhury
Copy link
Member Author

Geant4 provide Geant4Config.cmake that allows to find Geant4. Do we still need FindGeant4?

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This PR now undoes a lot of recent changes in DAGMC. It needs additional cleanup

Comment on lines 814 to 822
#if MOAB_VERSION_MAJOR == 5 && MOAB_VERSION_MINOR > 2
// find a which volume contains the current point
ErrorCode DagMC::find_volume(const double xyz[3], EntityHandle& volume,
const double* uvw) {
ErrorCode rval = ray_tracer->find_volume(xyz, volume, uvw);
return rval;
}
#endif

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be removed

@ahnaf-tahmid-chowdhury
Copy link
Member Author

Adding #860 for history

@ahnaf-tahmid-chowdhury
Copy link
Member Author

#34 5.300 /root/build_dir/dagmc/src/pyne/pyne.cpp/root/build_dir/dagmc/src/dagmc/DagMC.cpp:816:18: error: out-of-line definition of 'find_volume' does not match any declaration in 'moab::DagMC'
  #34 5.305 ErrorCode DagMC::find_volume(const double xyz[3], EntityHandle& volume,
  #34 5.306                  ^~~~~~~~~~~
  #34 5.308 :12283:74: warning: implicit conversion from 'const Json::Int64' (aka 'const long long') to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion]

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

As I've mentioned in a few places, the way you have reconstructed this PR has removed some important updates that came from other PRs that happened in between. Normally, if you hadn't deleted your previous branch, we could just rely on a git rebase, but this will take a little more care to get right.

.github/workflows/docker_publish.yml Outdated Show resolved Hide resolved
.github/workflows/docker_publish.yml Outdated Show resolved Hide resolved
.github/workflows/docker_publish.yml Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@

# Global ARGS set before the first build stage are accessable by all build stages
ARG EMBREE_BRANCH='v3.6.1'
ARG geant4_version=10.7.4
ARG geant4_version=11.1.2
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need/want to update the default yet - I'll think about it as I review the rest of the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I will be testing with both

EntityHandle DagMC::entity_by_id(int dimension, int id) const {
EntityHandle DagMC::entity_by_id(int dimension, int id) {
Copy link
Member

Choose a reason for hiding this comment

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

this is a regression from another PR that added the const - please remove this change

Comment on lines 567 to 563
inline EntityHandle DagMC::entity_by_index(int dimension, int index) const {
inline EntityHandle DagMC::entity_by_index(int dimension, int index) {
Copy link
Member

Choose a reason for hiding this comment

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

ibid

Comment on lines 573 to 569
inline int DagMC::index_by_handle(EntityHandle handle) const {
inline int DagMC::index_by_handle(EntityHandle handle) {
Copy link
Member

Choose a reason for hiding this comment

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

ibid

Comment on lines 578 to 574
inline unsigned int DagMC::num_entities(int dimension) const {
inline unsigned int DagMC::num_entities(int dimension) {
Copy link
Member

Choose a reason for hiding this comment

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

ibid

Comment on lines 15 to 16
require_density(require_density_present),
logger(verbosity) {
logger(verbosity),
require_density(require_density_present) {
Copy link
Member

Choose a reason for hiding this comment

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

This is an important change from another PR

Comment on lines +33 to +37
// retrieve the map
#ifdef GEANT4_GT_10_6
using MeshScoreMap = G4VScoringMesh::MeshScoreMap;
#endif

Copy link
Member

Choose a reason for hiding this comment

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

wasn't this already in an earlier PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I recall, this should be required for >10.6 and It may need to be available in earlier PR.

@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury marked this pull request as ready for review November 20, 2023 12:44
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

I think this is the only problem. Let's see what happens when the actions run on your account after we fix this.

cmake/FindGeant4.cmake Outdated Show resolved Hide resolved
This was referenced Nov 30, 2023
@ahnaf-tahmid-chowdhury
Copy link
Member Author

@gonuke docker build is passing now

@pshriwise pshriwise merged commit fa54fff into svalinn:develop Dec 7, 2023
3 of 12 checks 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.

3 participants