Make CollectionCollector collections optional #1843
Conversation
Capybara summary for PR 1843
|
|
I'm curious what others think, but I wonder if this adds unmanageable complexity to plugins. They are supposed to be self-contained and "default constructable" but requiring the DD4hep_service means we need to load one plugin before another. We don't resolve this automatically though, so every so often someone will have to manually reorder the plugin list... Not that I have a better solution here. Maybe we need to finally figure out optional inputs instead of failing when they are absent. |
I'm pretty sure the most recent JANA2 version will allow the optional inputs @nathanwbrei? What was the reason we have our own JOmniFactory rather than directly using the JANA2 versions? |
OT I tried to inherit more of the upstream JOmniFactory functionality, and there are at this point a few issues, primarily the absence of ParameterRef and Resource support upstream. The upstream JOmniFactory also doesn't support our Process signatures (which are JANA2 and JEvent agnostic). Nothing major, but unlikely we'll be able to get rid of |
for more information, see https://pre-commit.ci
|
@wdconinc copying across the minimal changes from main JANA2 for optional collections appears to work for me in the small tests I've done. |
Might have jumped the gun, I had just been testing the compilation and forgotten to reinstall... |
simonge
changed the title
|
I'm pretty sure it works properly now. There may be details beyond my understanding which should be fixed. |
|
Looks like it crashes. |
…articles-with-no-calorimeters-in-configuration
for more information, see https://pre-commit.ci
src/extensions/jana/JOmniFactory.h
Outdated
| void GetCollection(const JEvent& event) { | ||
| try { | ||
| m_data = event.Get<T>(this->collection_names[0]); | ||
| } catch (const std::exception& e) { |
There was a problem hiding this comment.
Can we narrow this to the specific exception when a collection is missing in the input and not the collection's algorithm made a boo-boo? I know, likely it's JException or bust, but I can dream, right?
There was a problem hiding this comment.
But this is just a rethrowing. I think motivation here was to provide nicer debug messages, not improve actual handling of errors.
Ah, I misremembered the context, there is a check for optionality, which affects control flow.
Also, in principle, we could have added such context via boost::exceptions.
There was a problem hiding this comment.
Regarding narrowing by type, it's probably JException for collection not found (probably, same exact exception is thrown when none of the factories succeeded to initialize).
There was a problem hiding this comment.
But we control the input layer through JEventSourcePodio. Does that help here? (On my phone so I'm not able to check in detail.)
There was a problem hiding this comment.
I don't what you have in mind here, we either have a type match or not in this clause. Generally, control over input layer doesn't help unless we can sort the dependency graph.
There was a problem hiding this comment.
From my point of view this can get dealt with in a future PR, but I'd like to ask we print out at least a warning when a collection is ignored (until we can be sure it is ignored for the right reasons). And we should create an issue to keep track of this need.
There was a problem hiding this comment.
That would probably result in a lot of log messages for incomplete configurations, where every event could flag up every missing collection.
Looking at the capybara report again I am now confused as to how tracking_only already has entries in the ReconstructedParticles collection. Something about the behaviour must be different to what I've been assuming.
There was a problem hiding this comment.
Can we agree that exceptions arising from "missing" collections are limited to JException?
| } catch (const std::exception& e) { | |
| } catch (const JException& e) { |
This will leave more general class std::exceptions to the status quo.
There was a problem hiding this comment.
Looking at the capybara report again I am now confused as to how tracking_only already has entries in the ReconstructedParticles collection. Something about the behaviour must be different to what I've been assuming.
Do you know of a specific piece that should fail when calorimeters are missing?
There was a problem hiding this comment.
Looking at the capybara report again I am now confused as to how tracking_only already has entries in the ReconstructedParticles collection. Something about the behaviour must be different to what I've been assuming.
Do you know of a specific piece that should fail when calorimeters are missing?
I have done a couple of extra tests on the main branch which show I can recover the behaviour of the tracking_only configuration with the ip6 configuration.
The difference appears to be if I request EcalClusters or all three of EcalEndcapPClusters,EcalBarrelScFiClusters,EcalEndcapNClusters in my output_collections list, it forces the creation of the empty collection(s) even if the inputs don't exist, these empty collections then exist enough to propagate through the factories to fill the ReconstructedParticles collection.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Apologies, but I'm catching up and I think I lost the thread here. Let me ask a few conceptual questions for my own understanding:
- The goal here is to create reconstructed particles without using any calorimeter inputs, i.e. to be able to run
MatchClusterswithout providing a cluster collection, right? - So how does the
CollectionCollectorfit into this? Is this a test case for making sure optional inputs work now?
If the answer is yes to (1), this seems to me to be the right path towards accomplishing this, though...
The original aim was as you suggested. My initial approach was to just copy what we've done for the tracking and test if the input hit collection exists in the geometry. This was never so allowing properly optional input collections has been implemented instead. An alternative/future modification would be to have the cluster input into A bit more context. I am testing some downstream software developed by a colleague which just uses the |
But right now this doesn't just test that, but also whether a collection that is added has failed to be constructed for any other reason. My concern in #1843 (comment) is that this would let these other reasons go unnoticed... |
I don't see that's any different than the current behaviour? The fact that there are no merged clusters doesn't stop ReconstructedParticles being created, it just silently stops it being populated. Other than just checking and understanding the output would we require upstream modifications the JANA2 exceptions or a different approach altogether to handle this properly? |
An empty collection is much more noticeable than a filled collection with some class of elements missing. The only reason we have caught some of these exception issues in the past month is because ReconstructedParticles was empty. I don't want to hold up this PR, but we should be aware of the pitfalls. |
Gotcha! Thanks for the clarification! That helps a lot! And in light of that, I'd second deferring additional work on std::exception vs. JException to a follow-up PR. I'm pretty happy with this as-is! |
…articles-with-no-calorimeters-in-configuration
There was a problem hiding this comment.
No objections from me. I think, we should also ping @nathanwbrei on this.
Briefly, what does this PR introduce?
Allows the creation of ReconstructedParticles using configurations which don't contain the calorimeters, e.g. epic_ip6.xml.
The selection of calorimeter hits included will probably need to change in the future.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No
Does this PR change default behavior?
No