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 update #924

Merged
merged 33 commits into from
Dec 7, 2023
Merged

Geant4 v11 update #924

merged 33 commits into from
Dec 7, 2023

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented Nov 30, 2023

Note: These changes were first proposed in #907 by @ahnaf-tahmid-chowdhury. In order for the docker build process to test smoothly, a new PR is being issued with all those changes, attributed to him, but without updating the standard linux_build_test.yml workflow. That workflow will require the new docker images to exist first.

Description

Introduce changes to support building against Geant4 v11.x

Motivation and Context

Some code API changes for Geant4 require some changes.

Changes

Supports build of newer versions of Geant4

Expected behavior

Since these changes should be backward compatible with v10 of Geant4, all tests should pass including testing on both new and old versions of Geant4

@gonuke
Copy link
Member Author

gonuke commented Nov 30, 2023

Mac testing will fail until we merge #922

@ahnaf-tahmid-chowdhury
Copy link
Member

I think, there is no need to install gcc manually. GitHub workflow already comes with built-in dev tools.

@gonuke
Copy link
Member Author

gonuke commented Dec 1, 2023

I'm trying to skip gcc in #922 right now

@gonuke gonuke mentioned this pull request Dec 1, 2023
@gonuke
Copy link
Member Author

gonuke commented Dec 1, 2023

I think, there is no need to install gcc manually. GitHub workflow already comes with built-in dev tools.

Thanks @ahnaf-tahmid-chowdhury - I've moved that change into this PR since it was such a small change.

Looking for a review here from @pshriwise @shimwell @makeclean @helen-brooks ?

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Thanks @ahnaf-tahmid-chowdhury and @gonuke for the update to a newer Geant4 version.

Mostly line comments on the Python script here.

.github/workflows/mac_build_test.yml Show resolved Hide resolved
@@ -70,7 +72,7 @@ def parse_arguments():
try:
fh = open(options.dag_file, "r")
except IOError:
print "Error: can\'t find DAG file"
print("Error: can't find DAG file")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("Error: can't find DAG file")
print("Error: can't find DAG file: '{}' ".format(options.dag_file))

Copy link
Member

Choose a reason for hiding this comment

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

To avoid the extra else block at the end:

try:
    fh = open(options.dag_file, 'r')
    fh.close()
except IOError:
    print("Error: can't find DAG file: '{}'".format(options.dag_file))

Comment on lines 132 to 133
print("There are no materials in the h5m file!!")
print("Assuming all volumes contain vacuum")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("There are no materials in the h5m file!!")
print("Assuming all volumes contain vacuum")
print("There are no materials in the h5m file.\n "
"Assuming all volumes contain vacuum")

@@ -534,7 +484,7 @@ dag_volume_names : list or dictionary of volume names
def _setup_dagspecific_headers(dag_volume_names, tallies_needed):

if not dag_volume_names:
print "dag volume names not assigned", sys.exc_info()[0]
print("dag volume names not assigned", sys.exc_info()[0])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("dag volume names not assigned", sys.exc_info()[0])
print("DAGMC volume names are not assigned", sys.exc_info()[0])

@@ -1680,7 +1630,7 @@ def _write_files(path, dictionary_files):
try:
fh = open(path+'/'+key, "w")
except IOError:
print "Error: "+file+" file exists"
print("Error: "+key+" file exists")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("Error: "+key+" file exists")
print("Error: {} file exists".format(key))

@@ -1724,7 +1677,7 @@ def _mkdir_p(path):
try:
os.makedirs(path)
except OSError:
print "Error: can\'t create directory, "+path
print("Error: can't create directory, "+path)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("Error: can't create directory, "+path)
print("Error: can't create directory '{}' ".format(path))


mat_tag = dag_geom.getTagHandle('NAME')
dag_vol_names = [dag_materials.keys()]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dag_vol_names = [dag_materials.keys()]
dag_vol_names = list(dag_materials.keys())

@@ -1707,9 +1657,12 @@ def _create_directory_structure(directory):
_mkdir_p(directory+'/src')
return

# Fix: Don't know what it does
"""
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to remove this function?

@@ -30,7 +30,11 @@ void ExN01UserScoreWriter::DumpAllQuantitiesToFile(const G4String& fileName,
G4cout << "Dumping mesh " << fScoringMesh->GetWorldName() << " to file"
<< G4endl;

// retrieve the map
// retrieve the map
Copy link
Member

Choose a reason for hiding this comment

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

These lines are also just above. We should only need this once. Might be a change inherited from the original PR.

@gonuke
Copy link
Member Author

gonuke commented Dec 7, 2023

Thanks for the review @pshriwise - I think I addressed everything.

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Thanks @gonuke!

@pshriwise pshriwise merged commit c82b52d into develop Dec 7, 2023
8 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