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 utility wrapper methods for easier access to detector geometry parameters #23

Merged
merged 7 commits into from
Sep 29, 2021

Conversation

dudarboh
Copy link
Member

@dudarboh dudarboh commented Sep 12, 2021

BEGINRELEASENOTES

  • Add generic MarlinUtil::getDetData for getting DDRec detector extension data from dd4hep.
  • Add dedicated getVXDData, getSITData, getFTDData, getTPCData, getSETData in MarlinUtil::ILD namespace which return DDRec extensions of corresponding detector elements of the ILD detector.
  • Fix warnings about catching exceptions by value in TrueJet_Parser

ENDRELEASENOTES

As discussed here #22, it is not a bad idea to have wrapper methods which would return extension of the detector elements, so user can have more intuitive access to the detector parameters.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Hi @dudarboh,

I think this goes into the right direction. The only thing that I would change is that instead of implementing the same function over and over again, I would implement one templated function that also takes the name of the detector as argument, and then simply call that one from the functions you have defined. As an additional bonus, you have then also defined a function that allows users to get more "exotic" extension data or to get extension data from detectors with a different name. Nevertheless, the main use cases are covered by the default functions you provide otherwise.

For the documentation, I think it is enough to refer to dd4hep from here. Ideally with a link to the actual documentation, or if that is not easily possible, just a sentence that refers the user to dd4hep for further information.

source/src/GeometryUtil.cc Outdated Show resolved Hide resolved
source/src/GeometryUtil.cc Outdated Show resolved Hide resolved
source/src/GeometryUtil.cc Outdated Show resolved Hide resolved
source/include/GeometryUtil.h Outdated Show resolved Hide resolved
@gaede
Copy link
Contributor

gaede commented Sep 14, 2021

Hi @dudarboh, this is nice. One problem though: the utility should be genric, i.e. work for every detector not only ILD. Can you split this up and add a dedicated ILDGeometryHelpers.h with the ILD specific template specializations ?

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the exception warnings 👍

source/include/ILDGeometryUtil.h Outdated Show resolved Hide resolved
dudarboh and others added 2 commits September 14, 2021 15:38
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
@tmadlener tmadlener linked an issue Sep 14, 2021 that may be closed by this pull request
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

@dudarboh is there anything from your side that is still missing? Otherwise, I would say we add a link to the dd4hep documentation of where more information of the returned classes can be obtained and we merge this.

@dudarboh dudarboh changed the title [WIP] Add utility wrapper methods for easier access to detector geometry parameters Add utility wrapper methods for easier access to detector geometry parameters Sep 28, 2021
@dudarboh
Copy link
Member Author

Hi @tmadlener,

nothing is missing at this point. I added documentation and removed WIP. I think this is ready for merging

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks.

@tmadlener tmadlener merged commit 8f9b5ed into iLCSoft:master Sep 29, 2021
@dudarboh dudarboh deleted the feature_getDetData branch November 9, 2021 19:02
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.

Suggestion for a method to return TPC radii
3 participants