FairModule IsSensitive checks medium property not volume name#1628
FairModule IsSensitive checks medium property not volume name#1628rlalik wants to merge 3 commits intoFairRootGroup:devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hi,
others might be more into the current thigns. So these are just my random comments.
Generally (I hope, we have that in our contribution guidelines?)
- Please update the copyright year on any changed files
- Add yourself to the CONTRIBUTORS list (possibly with OCRID)
- By no longer calling the old virtual function, this code is a breaking change. I don't know, whether the main authors are okay with that?
- Stop using those ROOT specific bool type. C++ has a good one since 20+ years.
Non virtual interface pattern:
- I'd really suggest to separate the calling and the customization points. That is:
- Introduce a new non-virtual interface. Maybe something like
IsVolumeSensitive(TGeoVolume const&). (implementation suggestions below) - Make your new virtual
IsVolumeSensitiveImpl(TGeoVolume const&)private (one can override private functions!) - Before calling any of these
TGeoVolume&functions, make sure that the volume is!= nullptr.
bool IsVolumeSensitive(TGeoVolume const& vol) const
{
if (IsVolumeSensitiveImpl(vol))
return true;
// Call old code, for legacy reasons:
return IsSensitive(vol.GetName());
}
|
@ChristianTackeGSI thanks for comments. I read Contributing guidelines, but somehow forgot to return to it after adjusting PR to guideline no. 1. How many (reasonable) users of FairRoot are at the moment beside CbmRoot and PandaRoot that I am aware of? The consequence of this PR would be:
|
|
Thanks for updating contributors!
I had the hope that the old legacy Generally speaking, I am hoping that @fuhlig1 or @karabowi have a much better understanding of the "business logic" / machinery and will comment on it. Nevertheless I would prefer private customization points (virtual functions) and non virtual interfaces (if needed). My one was just an idea.
The problem here: deprecations in C++ only work for the caller. So anyone calling a deprecated function will get a warning. overriding a deprecated function does not create a warning. I don't know of any way to output a warning for just overriding a virtual function. Sadly. So either we set the function to
Very interesting idea! Maybe open another PR to change the guidelines? I'd certainly give it an upvote! |
rlalik
force-pushed
the
issensitive_medium
branch
from
3af01ab to
d336cce
Compare
…ume name This is faster way of checking the sensitivity property however medium needs to have proper sensitivity flag set.
By ,arking old IsSensitive function as final, all deriving classes will generate compilation error.
rlalik
force-pushed
the
issensitive_medium
branch
from
d336cce to
b3c2c2a
Compare
|
@rlalik I had a look at your PR and if I were the only one to decide, I would straight reject it. The rationale behind (I had also a short discussion with @MohammadAlTurany ) is that accepting this PR would totally screw up the sensitive detectors in all experiments, including CBM. To summarize: this PR should wait for @fuhlig1 |
Thanks @karabowi for this opinion and your concern. I am aware of the impact this PR has. However I don't think the solution will be very difficult. In case of CBM, one need to update material definitions in the For cbmroot_geometry this is only twelve active sensors (perhaps medium as well, can be less if different sensors use same medium, magnet is always non-sensitive):
I wouldn't say that amount of work is here massive. Don't know about PANDA. And what are the other user of FairRoot. |
|
Alice? R3B? Ship? Assume only one detector in CBM that uses a certain medium as active or passive using FairRoot's naming scheme. We will discuss it. |
|
Hi, in R3BRoot we don’t use that function. |
string based matching is sooo inefficient. All you guys make a fuzz about C++ and performance, and then you're going for interpreter style tooling? Okay, enough stupid ranting by me. |
|
Okay, alternative new "interface" function: bool IsVolumeSensitive(TGeoVolume const& vol) const
{
// Ask old / legacy stuff.
if (IsSensitive(vol.GetName()))
return true;
// Call new
return IsVolumeSensitiveImpl(vol);
}It first tries the old path. So that all the legacy stuff still works. This gives us a transition without a breaking change? Then let's add a note to the CHANGELOG, that people should migrate. Then we could add a Warning to the above like this if (IsSensitive(vol.GetName())) {
LOG(warning) << "FairModule::IsSensitive is deprecated, please override IsVolumeSensitiveImpl()!";
return true;
} |
| v->RegisterYourself(); | ||
| } | ||
| if ((this->InheritsFrom("FairDetector")) && IsSensitive(v->GetName())) { | ||
| if ((this->InheritsFrom("FairDetector")) && IsSensitive(v)) { |
There was a problem hiding this comment.
this->InheritsFrom("FairDetector")BTW: Can we please (in another PR!)
- Move this out of the loop? It's run each time inside a loop!
- Replace this by a
(dynamic_cast<FairDetector>(this) != nullptr)?
| * | ||
| * See Bool_t IsSensitive(const std::string& name) for deprecation notice. | ||
| */ | ||
| virtual bool IsSensitive(TGeoVolume* vol) { return (vol->GetMedium()->GetParam(0) == 1.0); } |
There was a problem hiding this comment.
IF I got some of the notes correctly, then this is a standard attribute of the medium?
The 0 looks like a magic constant? Is there some const / define available for this?
Thinking about my backward compatible request, maybe the current default implementation should be return false;?
And in a later update we could use this one?
Yes, this means that everyone who wants to use this customization point would need to implement this code. But it would maybe help with PR acceptance?
There was a problem hiding this comment.
Geant 3 defienes ISVOL flag:
And we use TGeoMedium which defines it in similar way as integer:
https://root.cern.ch/doc/master/TGeoMedium_8cxx_source.html#l00064
There is no constant/enum defined, just 0 - not sensitive, > 0 - sensitive. TGeoMedium stores parameters as floating points. The proper check could be actually vol->GetMedium()->GetParam(0) > 0.0.
So, as I also wrote it before, this is bi-state, either volume is senstivie or non-sensitive. Thats way false as a return is also a valid result of the check. we would need tri-state return value to check whether the function was implemented good candidate would be std::optional with {} as return. But this also requires to change function signature.
Other idea - using exceptions. Throw in the base function and catch it, or the derived function returns valid value. Executing the string-based check will give some deprecation message, and perhaps full removal in the future (?).
There was a problem hiding this comment.
I meant, whether there's a constant for the 0 in ->GetParam(0)? This really feels like a magic constant. I don't like those.
There was a problem hiding this comment.
Yeah, as you can find in line 84 of this code https://root.cern.ch/doc/master/TGeoMedium_8cxx_source.html#l00064, it is just magic number.
OFFTOPIC: Alice uses FairRoot? Don't they have their own AliRoot? I'm just surprised. |
@jose-luis-rs Out of curiosity, how do you test for sensitive detector? This function is called anyway. Do you have separate checks in |
|
About the fallback / etc: the current So if you want to return true at all, you need to override one of the two? |
|
Yes, you cannot use Or other case, we moved to the new interface and the volume is sensitive. At some point one decided to make it insensitive but the old override of the I think this is bug prone and it is best to get rid of the old one entirely. |
|
Let's leave this for @fuhlig1. |
5 participants
This is faster way of checking the sensitivity property however medium needs to have proper sensitivity flag set.
See the discussion in this MR: https://git.cbm.gsi.de/CbmSoft/cbmroot_geometry/-/merge_requests/342#note_60660
@fuhlig1 is this is something you wanted to have?
I tried to check all the modified examples and tests whether they are still correct (all tests are passing), I need someone who is more familiar to check as well.
But it is not easy to test against cbmroot
Checklist: