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 ParT and UParT SONIC producer second pull request #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

y19y19
Copy link

@y19y19 y19y19 commented Jun 21, 2024

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Before submitting your pull requests, make sure you followed this checklist:

constexpr unsigned n_npf_accept = 25;
constexpr unsigned n_sv_accept = 5;

const std::map<InputFeatures, unsigned int> N_InputFeatures{

Choose a reason for hiding this comment

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

this should be nInputFeatures to follow the CMS naming rules: http://cms-sw.github.io/cms_coding_rules.html#2--naming-rules-1
(similarly for the other similar variables below)

constexpr unsigned n_sv_accept = 5;

const std::map<InputFeatures, unsigned int> N_InputFeatures{
{kChargedCandidates, 16},

Choose a reason for hiding this comment

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

these could actually be vectors rather than maps, since the enum is ordered and dense. that would speed up some of the operations in the producers.

const unsigned int target_n_npf = std::clamp(max_n_npf_counter, (unsigned int)1, (unsigned int)parT::n_npf_accept);
const unsigned int target_n_vtx = std::clamp(max_n_vtx_counter, (unsigned int)1, (unsigned int)parT::n_sv_accept);

const std::map<parT::InputFeatures, unsigned int> target_n {

Choose a reason for hiding this comment

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

this can also be a vector (etc.)


if (taginfo.features().is_filled) {
for (std::size_t flav_n = 0; flav_n < flav_names_.size(); flav_n++) {
(*(output_tags[flav_n]))[jet_ref] = outputs_from_server[jet_n][flav_n];

Choose a reason for hiding this comment

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

this can be:

(*(output_tags[flav_n]))[jet_ref] = taginfo.features().is_filled ? outputs_from_server[jet_n][flav_n] : -1.0;

so that the for loop doesn't have to be repeated

std::vector<float> inputs_UparT(const btagbtvdeep::SecondaryVertexFeatures& sv_features, UparT::InputFeatures ifeature);

template<class parT_features>
void parT_tensor_filler(cms::Ort::FloatArrays& data, const parT::InputFeatures ifeature, const std::vector<parT_features>& features, const unsigned int max_n, const float*& start, unsigned offset) {

Choose a reason for hiding this comment

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

make sure to run these commands before submitting the official PR:

scram b code-checks
scram b code-format

const unsigned int target_n_npf = (unsigned int)UparT::n_npf_accept;
const unsigned int target_n_vtx = (unsigned int)UparT::n_sv_accept;

const std::map<UparT::InputFeatures, unsigned int> target_n {

Choose a reason for hiding this comment

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

can be vector


if (taginfo.features().is_filled) {
for (std::size_t flav_n = 0; flav_n < flav_names_.size(); flav_n++) {
(*(output_tags[flav_n]))[jet_ref] = outputs_from_server[jet_n][flav_n];

Choose a reason for hiding this comment

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

see above comment

timeout = cms.untracked.uint32(300),
mode = cms.string("Async"),
modelName = cms.string("particletransformer_AK4"), # double check
modelConfigPath = cms.FileInPath("RecoBTag/Combined/data/models/particletransformer_AK4/config.pbtxt"), # this is SONIC, not currently in the CMSSW, so the models/ will be copied to this location privately

Choose a reason for hiding this comment

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

this comment should be removed for the official PR

timeout = cms.untracked.uint32(300),
mode = cms.string("Async"),
modelName = cms.string("unifiedparticletransformer_AK4"), # double check
modelConfigPath = cms.FileInPath("RecoBTag/Combined/data/models/unifiedparticletransformer_AK4/config.pbtxt"), # this is SONIC, not currently in the CMSSW, so the models/ will be copied to this location privately

Choose a reason for hiding this comment

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

this comment should be removed for the official PR

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.

2 participants