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

fix: set materials-map string constant with path to materials map #494

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Aug 3, 2023

Briefly, what does this PR introduce?

This works with eic/EICrecon#826 to set the materials-map string constant in the geometry so we can avoid having to assume that calibrations/materials-map.cbor is where we want the materials map to be searched for. The latter approach leads to race conditions when the same geometry directory is used by multiple running processes which keep updating the calibrations/materials-map.cbor file with what they want, only to read it later when it might have changed again already.

What kind of change does this PR introduce?

  • Bug fix (issue: possible cause for craterlake eicweb reconstruction benchmark failures)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators: @ShujieL @bschmookler

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

Yes. The materials map will be written to a different file name. An updated EICrecon will be required to pick it up automatically. Alternatively, you can explicitly pass the option e.g. -Pacts:MaterialMap=calibrations/materials-map.cbor.

@veprbl
Copy link
Member

veprbl commented Aug 3, 2023

I have a couple questions:

  1. Can we add the value from the plugin itself, using https://github.com/AIDASoft/DD4hep/blob/ad3c96d6ff95dca60d510fe78f0cffa25068c81c/DDCore/include/DD4hep/DetectorImp.h#L344 The downside is that it possibly creates a precedent of a constant not defined in xml.
  2. Instead of name suffixes like -arches would it make more sense to use content-addressed filenames (include file hash in the name)?

@wdconinc
Copy link
Contributor Author

wdconinc commented Aug 4, 2023

Can we add the value from the plugin itself

I think you already hint at the reason: it puts more stuff inside the plugins, which I'd rather not do because it hides it from where users would naturally go and look for this info.

veprbl
veprbl previously approved these changes Aug 5, 2023
@wdconinc wdconinc force-pushed the explicit-materials-map-constant branch from bc846f8 to 1ebae8f Compare August 5, 2023 13:56
@Chao1009 Chao1009 force-pushed the explicit-materials-map-constant branch from 1ebae8f to c34bba3 Compare September 5, 2023 19:53
veprbl added a commit to eic/EICrecon that referenced this pull request Sep 18, 2023
The eic/epic#494 is not merged at this time. Let's update that one to provide both constants, but going forward use proper meaning one.
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

compact/tracking/definitions.xml Outdated Show resolved Hide resolved
compact/tracking/definitions_arches.xml Outdated Show resolved Hide resolved
compact/tracking/definitions_craterlake.xml Show resolved Hide resolved
wdconinc pushed a commit to eic/EICrecon that referenced this pull request Sep 19, 2023
The eic/epic#494 is not merged at this time. Let's update that one to provide both constants, but going forward use proper meaning one.
wdconinc pushed a commit to eic/EICrecon that referenced this pull request Sep 19, 2023
The eic/epic#494 is not merged at this time. Let's update that one to provide both constants, but going forward use proper meaning one.
github-merge-queue bot pushed a commit to eic/EICrecon that referenced this pull request Sep 19, 2023
…992)

The eic/epic#494 is not merged at this time.
Let's update that one to provide both constants, but going forward use
proper meaning one.
@rahmans1
Copy link
Contributor

rahmans1 commented Feb 7, 2024

@wdconinc Is this ready to merge?

@wdconinc wdconinc force-pushed the explicit-materials-map-constant branch from 92a561e to 40b6450 Compare February 7, 2024 04:12
@wdconinc
Copy link
Contributor Author

wdconinc commented Feb 7, 2024

Nah, let's keep it for next month, if at all.

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

backward compatibility not needed anymore, if we have forward compatibility?

compact/tracking/definitions.xml Outdated Show resolved Hide resolved
compact/tracking/definitions_arches.xml Outdated Show resolved Hide resolved
compact/tracking/definitions_craterlake.xml Outdated Show resolved Hide resolved
veprbl
veprbl previously approved these changes Feb 12, 2024
@wdconinc
Copy link
Contributor Author

auto-merge was automatically disabled April 8, 2024 22:31

Merge queue setting changed

@veprbl
Copy link
Member

veprbl commented Apr 16, 2024

  01:44:29    epic.getDete   INFO      Adding material from /home/runner/work/epic/epic/scripts/material_map/calibrations/materials-map.cbor
  Traceback (most recent call last):
    File "/home/runner/work/epic/epic/scripts/material_map/material_validation_epic.py", line 46, in <module>
      detector, trackingGeometry, decorators = epic.getDetector(args.xmlFile, args.matFile)
    File "/home/runner/work/epic/epic/scripts/material_map/epic.py", line 33, in getDetector
      matDeco = acts.IMaterialDecorator.fromFile(
    File "/usr/local/python/acts/__init__.py", line 61, in _decoratorFromFile
      return ActsPythonBindings.JsonMaterialDecorator(
  RuntimeError: Unable to open input JSON material file: calibrations/materials-map.cbor
  ./action_payload.sh: Error on line 12: ./run_material_map_validation.sh
  /home/runner/work/_actions/eic/run-cvmfs-osg-eic-shell/main/setup-eic-shell.sh: Error on line 28: $THIS/run-linux.sh

@wdconinc
Copy link
Contributor Author

Right, that's a place where I've hard-coded it ... sigh.

auto-merge was automatically disabled May 7, 2024 16:33

Merge queue setting changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants