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 possibility to invent seeding stubs coordinates before DR #170

Merged
merged 8 commits into from
Jun 24, 2022

Conversation

sarafiorendi
Copy link

PR description:
Include code to enable the setting of seeding stubs coordinates to those inferred from the tracklet trajectory, before the DR step.
Performance shown here

@sarafiorendi sarafiorendi marked this pull request as ready for review June 17, 2022 08:48
@sarafiorendi sarafiorendi requested a review from a team June 17, 2022 08:49
bool isSeedingStub(int, int, int);
std::string l1tinfo(const L1TStub*, std::string);
std::pair<int, int> findLayerDisk(const Stub*);
std::vector<double> getInventedCoords(unsigned int, const Stub*, Tracklet*);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comments explaining what these two "invented" functions do.

Copy link
Author

Choose a reason for hiding this comment

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

done

std::pair<int, int> findLayerDisk(const Stub*);
std::vector<double> getInventedCoords(unsigned int, const Stub*, Tracklet*);
std::vector<double> getInventedCoordsExtended(unsigned int, const Stub*, Tracklet*);
std::vector<const Stub*> seedStubCoordsFromTracklet(unsigned int, Tracklet*, std::vector<const Stub*>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment explaining what seedStubCoordsFromTracklet does. Also the name of this function is misleading. It doesn't return the coordinates. It returns the invented stubs.

Copy link
Author

Choose a reason for hiding this comment

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

done, and name changed

Stub* invent_stub_ptr = new Stub(*thisStub);
const L1TStub* L1stub = thisStub->l1tstub();

L1TStub invent_L1stub(L1stub->DTClink(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a long-winded way to copy an L1TStub. Can't you just profit from L1TStub's default copy constructor and do:
L1TStub invent_L1TStub = L1stub;
invent_L1TStub.setCoords(x,y,z);
where you also need to add function L1TStub::setCoords(x,y,z);

Copy link
Author

Choose a reason for hiding this comment

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

updated

double stub_z_invent = inv_r_z_phi[1];

Stub* invent_stub_ptr = new Stub(*thisStub);
const L1TStub* L1stub = thisStub->l1tstub();
Copy link
Collaborator

Choose a reason for hiding this comment

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

All variable names in CMSSW must start with a lower case letter.

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -13,6 +13,7 @@
asciiFileName = cms.untracked.string(""),
Extended = cms.bool(False),
Reduced = cms.bool(False),
InventStubs = cms.bool(False),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this parameter needs to be in Tracklet_cfi.py, and not just specified in Settings.h?

Copy link
Author

Choose a reason for hiding this comment

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

yes so that it can be changed w/o compiling the code

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, I suggest removing it from Tracklet_cfi.py. The same argument would apply to every other parameter in Settings.h. The problem with putting params in Tracklet_cfi.py is that the value specified in Settings.h is then ignored, which tends to confuse people. Ahd we won't change this parameter of yours very often.
(The use of Settings.h is ugly, and forced on us by the request of some members of L1TrkAlgo that the code can also run outside CMSSW. I hope we can move away from this in the Future emulation.)

@tomalin
Copy link
Collaborator

tomalin commented Jun 23, 2022

I suggest setting inventStubs_=true, since whilst it is false, we are simulating an algo that we don't know how to implement in FW.

Copy link
Collaborator

@tomalin tomalin left a comment

Choose a reason for hiding this comment

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

Looks good

@tomalin tomalin merged commit 578048a into L1TK-dev-12_5_0_pre2 Jun 24, 2022
tomalin pushed a commit that referenced this pull request Nov 24, 2022
* add possibility to invent stubs

* fix missing index in l1tstub

* apply code format

* updated function names and description,update invent l1tstub creation

* apply code format

* remove parameter from tracklet_cfi

* change default invent option to true

* code format
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