fix: Cluster factory factory so cluster matching has associations #666
Conversation
|
Net lines of code: -1500... |
|
It's so satisfying when you can remove huge pieces of redundant code like that! |
|
And it will also work for all other calorimetry factories... In a different PR. |
|
I, too, appreciate the effort. A bit concerned about the size of the diff to review, though :) |
wdconinc
force-pushed
the
cluster-factory-factory
branch
from
e6e8fa6 to
a44f779
Compare
|
Thanks for this massive effort! From reading the code it seems to be ok and certainly removes duplication. Have you run any explicit tests for any of the calorimeters to test that you get what you expect on the cluster level, if so could you list them in the PR please. |
|
Ran out of time to do a full test. The test I did do was the proof of concept implementation on scfi clusters only, and that resulted in the correct cluster distributions and non-empty associations. Extending this to all cluster factories resulted in failing tests, which makes a pre/post comparison premature. First this needs some debugging of the segfaults (i.e. why does podio try to write collections to the frame from multi-factories that should have been disabled due to lack of necessary hit collections, even when activating the successful_collections in the podio processor @nathanwbrei). Someone with a debug build of podio outside of eic-shell will have an easier time identifying which collection ends up with a nullptr. If we want this by June (and I think we do) someone would need to take this over from here since I won't be able to complete it until in June. A next step, as soon as this is demonstrated to work or even in parallel, may be a refactor for similar code reduction on the other calorimetry factories which are all also just duplicated code. What's needed there is conversion to using the config mixin on the algorithm side, and removing the concrete factories in separate translation units in favor of a templated solution. Finally, I don't think there is a way around the using varargs constructor to pick up the required constructors, by maybe someone can find a way around it by using a different construct. |
wdconinc
force-pushed
the
cluster-factory-factory
branch
from
a44f779 to
fa992e4
Compare
…ergy/position clusters)
… inconsistency and match the tag name
|
I see some odd behavior. When I run brycecanyon configuration with main and with this PR, new configuration options appear for:
But some collections disappear from the output file:
I don't understand how to reconcile this. |
|
There is no SciGlass and HcalEndcapP in brycecanyon. These collections should not have been present before, but a workaround was to write them anyway (see "TODO: NWB: Re-enable me" at https://github.com/eic/EICrecon/pull/666/files#diff-08c369ce77f61e0738a1b6bea175ebb10a4df985a97ced435296883bcca0992cL271). So, that is intended behavior as part of fixing the treatment of collections with missing dependencies. |
|
Okay. I agree, collections should not be there in the first place. I'm not able to see why old version doesn't produce parameters, but, at least, it's good that it works now. |
The wordiness of the current factory-factory,
```cpp
class Cluster_factory_EcalBarrelSciGlassTruthClusters: public
CalorimeterClusterRecoCoG_factoryT<Cluster_factory_EcalBarrelSciGlassTruthClusters>
{
public:
template <typename... Args>
Cluster_factory_EcalBarrelSciGlassTruthClusters(Args&&... args)
:
CalorimeterClusterRecoCoG_factoryT<Cluster_factory_EcalBarrelSciGlassTruthClusters>(std::forward<Args>(args)...)
{ }
};
```
has been bothering me since #666. This always seemed like it should be
possible to express it more elegantly, with a `using` or so.
However, the naive approach
```cpp
using Cluster_factory_EcalBarrelSciGlassTruthClusters =
CalorimeterClusterRecoCoG_factoryT<Cluster_factory_EcalBarrelSciGlassTruthClusters>;
```
fails out of the gate since you can't use the lhs in the rhs.
This PR introduces an approach that does allow the `using` approach to
work, and it actually makes it even snappier:
```cpp
using Cluster_factory_EcalBarrelSciGlassTruthClusters =
CalorimeterClusterRecoCoG_factoryT<>;
```
(because why repeat yourself).
This doesn't fix #628...
…#784) ### Briefly, what does this PR introduce? The wordiness of the current factory-factory, ```cpp class Cluster_factory_EcalBarrelSciGlassTruthClusters : public CalorimeterClusterRecoCoG_factoryT<Cluster_factory_EcalBarrelSciGlassTruthClusters> { public: template <typename... Args> Cluster_factory_EcalBarrelSciGlassTruthClusters(Args&&... args) : CalorimeterClusterRecoCoG_factoryT<Cluster_factory_EcalBarrelSciGlassTruthClusters>(std::forward<Args>(args)...) { } }; ``` has been bothering me since #666. This always seemed like it should be possible to express it more elegantly, with a `using` or so. However, the naive approach ```cpp using Cluster_factory_EcalBarrelSciGlassTruthClusters = CalorimeterClusterRecoCoG_factoryT<Cluster_factory_EcalBarrelSciGlassTruthClusters>; ``` fails out of the gate since you can't use the lhs in the rhs. This PR introduces an approach that does allow the `using` approach to work, and it actually makes it even snappier: ```cpp using Cluster_factory_EcalBarrelSciGlassTruthClusters = CalorimeterClusterRecoCoG_factoryT<>; ``` (because why repeat yourself). This doesn't fix #628...
This applies the #666 treatment to the CalorimeterHitsMerger algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
This applies the #666 treatment to the CalorimeterHitsMerger algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
This applies the #666 treatment to the CalorimeterHitsMerger algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
### Briefly, what does this PR introduce? This applies the #666 treatment to the CalorimeterHitDigi algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - update the unit tests that are affected, - create a templated factory-factory to use in the plugins, - replace all old explicit RawCalorimeterHit factories with the new factory-factory. ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [x] 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 - [x] Changes have been communicated to collaborators @veprbl @nathanwbrei ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? No. --------- Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
This applies the #666 treatment to the CalorimeterHitReco algorithms (on top of #794): - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
This applies the #666 treatment to the CalorimeterHitsMerger algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
This applies the #666 treatment to the CalorimeterHitsMerger algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
This applies the #666 treatment to the CalorimeterTruthClustering algorithms: - put all configuration in a single configuration struct for use with WithPodConfig mixin, - create a templated factory-factory to use in the plugins, - replace all old explicit CalorimeterHit factories with the new factory-factory.
Briefly, what does this PR introduce?
In 23.05.1 we realized that cluster matching (though now implementated) doesn't actually work yet due to the absence of cluster associations being produced in clustering. One could fix this in 17 factories (#636) but I really didn't want to do that so here's an approach that reduces duplicate code (but otherwise is similar enough, with the exception of adding configuration structures).
Todo:
Closes: #636
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?
Now associations will be written too.