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.

Sign up for GitHub

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

Jump to bottom

Fix features order #29799

Merged
merged 3 commits into from May 13, 2020
Merged

Fix features order #29799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80bda12
Fix order of features in input to inference
rovere May 8, 2020
da02f90
Fix energy scale with fractions at inference
rovere May 10, 2020
eda2df4
Run code-format
rovere May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions RecoHGCal/TICL/plugins/PatternRecognitionbyCA.cc
View file
Open in desktop
Expand Up @@ -333,9 +333,9 @@ void PatternRecognitionbyCA::energyRegressionAndID(const std::vector<reco::CaloC
float *features = &input.tensor<float, 4>()(i, j, seenClusters[j], 0);

// fill features
Copy link
Contributor

Choose a reason for hiding this comment

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

This "interface" is extremely error prone (as this bug just demonstrated).
Whoever approved this code for CMSSW clearly was not doing their job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the original sin is #27917.

What are the plans to fix the use of anonymous, unordered list for passing parameters to tensorflow or other ML engines ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fwyzard this is a good point. Maybe the framework should add a data structure like edm::featureMap that enforces a schema and has interfaces to output in various ML formats.

(Though, this would only prevent one of the two problems addressed by this PR. No framework feature can specify whether or not energy should be divided by vertex multiplicity...)

Copy link
Contributor

Choose a reason for hiding this comment

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

(Though, this would only prevent one of the two problems addressed by this PR. No framework feature can specify whether or not energy should be divided by vertex multiplicity...)

:-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #29818 to follow this

*(features++) = float(cluster.energy() / float(trackster.vertex_multiplicity(k)));
*(features++) = float(std::abs(cluster.eta()));
*(features++) = float(cluster.phi());
*features = float(cluster.energy());
*(features) = float(cluster.phi());

// increment seen clusters
seenClusters[j]++;
Expand Down
6 changes: 3 additions & 3 deletions RecoHGCal/TICL/plugins/TrackstersMergeProducer.cc
View file
Open in desktop
Expand Up @@ -287,7 +287,7 @@ void TrackstersMergeProducer::produce(edm::Event &evt, const edm::EventSetup &es
<< track.eta() << ", " << track.phi() << std::endl;
}
result->push_back(t);
usedTrackstersTRK[tracksterTRK_idx] = 1;
usedTrackstersTRK[tracksterTRK_idx] = true;
tracksterTRK_idx++;
}
}
Expand Down Expand Up @@ -440,9 +440,9 @@ void TrackstersMergeProducer::energyRegressionAndID(const std::vector<reco::Calo
float *features = &input.tensor<float, inputDimension>()(i, j, seenClusters[j], 0);

// fill features
*(features++) = float(cluster.energy() / float(trackster.vertex_multiplicity(k)));
*(features++) = float(std::abs(cluster.eta()));
*(features++) = float(cluster.phi());
*features = float(cluster.energy());
*(features) = float(cluster.phi());

// increment seen clusters
seenClusters[j]++;
Expand Down