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 TracksFilter.cc #21241

Closed
wants to merge 3 commits into from
Closed

add TracksFilter.cc #21241

wants to merge 3 commits into from

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented Nov 9, 2017

EDFilter to be used in the cosmics during collision (CDC) trigger @HLT
as far as I know, indeed, there are not available such event filters

this makes use of the logical expression used by many selectors for selecting the tracks
and then it requests a minimum number of such tracks

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21241/1904

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-21241/1904/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-21241/1904/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

The code-checks are being triggered in jenkins.

@mtosi
Copy link
Contributor Author

mtosi commented Nov 9, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21241/1905

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24322/console Started: 2017/11/09 17:35

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

A new Pull Request was created by @mtosi (mia tosi) for master.

It involves the following packages:

HLTrigger/special

@Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2017

Comparison job queued.

@fwyzard
Copy link
Contributor

fwyzard commented Nov 21, 2017

Thanks @makortel , this actually works:

$ cat CommonTools/RecoAlgos/plugins/TrackSelectorCountFilter.cc

#include "CommonTools/UtilAlgos/interface/ObjectCountFilter.h"
#include "CommonTools/UtilAlgos/interface/StringCutObjectSelector.h"
#include "DataFormats/TrackReco/interface/Track.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"

using TrackSelectorCountFilter = ObjectCountFilter<reco::TrackCollection, StringCutObjectSelector<reco::Track, false>>::type;

// declare this template instantiation as a framework plugin
#include "FWCore/Framework/interface/MakerMacros.h"
DEFINE_FWK_MODULE(TrackSelectorCountFilter);

However it has a big problem: this does not implement the module's fillDescriptions method, and so no cfi file is generated; meaning that this filter will not be available for use at HLT - which was the whole point.

Also, why do we have two files called StringCutObjectSelector.h under CommonTools/Util and CommonTools/UtilAlgos ?

@makortel
Copy link
Contributor

However it has a big problem: this does not implement the module's fillDescriptions method, and so no cfi file is generated; meaning that this filter will not be available for use at HLT - which was the whole point.

Since we've hit this wall already before, I decided to try to craft a prototype of fillDescriptions for the generic selector infrastructure, and ended up with #21430. With that PR and the FillDescriptionTraits shown in the PR description placed after using TrackSelectorCountFilter = ... in @fwyzard's code snippet from #21241 (comment), a cfi file is generated.

@slava77
Copy link
Contributor

slava77 commented Nov 22, 2017 via email

@makortel
Copy link
Contributor

@slava77

It should still be possible to implement the fillDescriptions for the
module as a part of full template specialization on top of what the
template brings by default, shouldn't it?

I'm not sure I follow. I agree that by specializing class templates one can, in general, do whatever. Which (class?) template are you talking about? (should we move the discussion to #21430?)

@fwyzard
Copy link
Contributor

fwyzard commented Nov 22, 2017

It should still be possible to implement the fillDescriptions for the
module as a part of full template specialization on top of what the
template brings by default, shouldn't it?

But that would defeat the purpose of using the template approach in the first place, no ?

@slava77
Copy link
Contributor

slava77 commented Nov 22, 2017 via email

@makortel
Copy link
Contributor

@slava77 Is #21443 along what you had in mind?

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2017 via email

@perrotta
Copy link
Contributor

This is just to notify here that #21430 was just merged in 10_0_X, so that every one interested in pushing forward this pull request can take profit of it

@perrotta
Copy link
Contributor

-1

  • Just to remove it from the list of open PRs for reco
  • It will get reviewed once restarted and re-coded according to the latest developments

@fabiocos
Copy link
Contributor

@mtosi is this PR still needed and is there any activity planned in the short term? Otherwise better to close it and resume when some work can be put on it

@mtosi
Copy link
Contributor Author

mtosi commented Feb 25, 2019

thanks for the ping, and sorry for this never ending PR
I do not have the time to work on this, now :(
=> I'm going to close this PR and I'll open a dedicated one at the proper time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet