search

lcfiplus / LCFIPlus

Flavor tagging code for ILC detectors
https://confluence.slac.stanford.edu/display/ilc/LCFIPlus
GNU General Public License v3.0
6 stars 19 forks source link

This fixs a problem when using an external jet collection. #40

Closed ryonamin closed 5 years ago

ryonamin commented 5 years ago

It required an external vertex collection whose name is the jet collection name with the suffix of "_vtx", which can not be changed from configuration file. Such vertex collection is not necessarily needed.

BEGINRELEASENOTES

ENDRELEASENOTES

jstrube commented 5 years ago

I'm not sure I understand this change. The readvtx has a default parameter, but we can explicitly over-write it. Isn't it better to call InitJetCollection with the right value for readvtx for the specific jet collection you're interested in, rather than changing the default for everybody?

ryonamin commented 5 years ago

Dear Jan, Thank you for your reply. yes, I would agree with you. The problem is, it seems that readvtx is called only internally and we can not control the arguments (to be confirmed by experts). I guess no one care about this because this part will be skipped when we use a jet collection produced by lcfiplus jet clustering. I think we can safely modify it because otherwise it always requires a vertex collection named "jet collection name" + "_vtx" and it is inconvenient, I think... I would like experts to check and this is why I made this pull request.

suehara commented 5 years ago

Dear Ryo,

I don't think we need _vtx collection since we could work with external jet clustering. in the original code. I have no time today, but I will check tomorrow.

Taikan

On Wed, 22 Aug 2018 00:02:37 -0700 Ryo Yonamine notifications@github.com wrote:

Dear Jan, Thank you for your reply. yes, I would agree with you. The problem is, it seems that readvtx is called only internally and we can not control the arguments (to be confirmed by experts). I guess no one care about this because this part will be skipped when we use a jet collection produced by lcfiplus jet clustering. I think we can safely modify it because otherwise it always requires a vertex collection named "jet collection name" + "_vtx" and it is inconvenient, I think... I would like experts to check and this is why I made this pull request.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/lcfiplus/LCFIPlus/pull/40#issuecomment-414932586

-- Taikan Suehara Department of Physics, Kyushu University suehara@phys.kyushu-u.ac.jp

suehara commented 5 years ago

I briefly checked this. It seems if we don't have _vtx collection it emits warning message and skip the reading (written in LCIOStorer::ReadVertices()). Do you want to suppress the warning message?

I don't think it's good to change default behavior just to suppress the warning since it may have side effects. If you really want to do so, I recommend to check if the _vtx collection exists or not at InitJetCollections() instead and call InitVertexCollections() only if _vtx collection exists. This should have essentially no side effects.

ryonamin commented 5 years ago

Thank you very much for checking. Probably I don't understand correctly the design philosophy. Why does it assume "_vtx" collection? When we supply an external jet collection, are we supposed to supply "_vtx" collection at same time??

suehara commented 5 years ago

No. Those _vtx collections (actually the collection in LCFIPlus data structure corresponding to the collections) are created at JetVertexRefiner if you run external jet clustering. You need to run JetVertexRefiner before flavor tagging, regardless of either you use internal or external jet clustering.

ryonamin commented 5 years ago

This problem happens even if we run JetVertexRefiner before flavor tagging. The corresponding part (InitJetCollections() etc.) is originated from event->Get(_jincolname.c_str(), _inputJets) that is at the beginning of JetVertexRefiner::process(). So the "_vtx" correction must be created before the event->Get(...) but I don't see it is like that... (this is why I got the messages, I think).

But anyway if this doesn't affect the other processes, I agree to keep as it is.

suehara commented 5 years ago

Then I will close this now. You can submit modifications if you prefer to suppress the message by adding existence check at InitJetCollection().