JeffersonLab / clas12-offline-software

CLAS12 Offline Software
10 stars 73 forks source link

Names for MC matching bank #1081

Closed baltzell closed 1 year ago

baltzell commented 1 year ago

Shall we simplify and cleanup the naming conventions used here:

https://github.com/JeffersonLab/clas12-offline-software/blob/abb98af76b877786a48698be9a03f9b89fb9adba/etc/bankdefs/hipo4/mc.json#L113-L126

If so, we should do it for the pass2 release. I'll propose:

  {
        "name": "MC::Matching",
        "group": 40,
        "item" : 6,
        "info": "Generated-Reconstructed Particle Matching",
        "entries": [
            {"name":"tid",          "type":"S",  "info":"MC::Particle.tid identifier"},
        {"name":"pindex",       "type":"S",  "info":"REC::Particle row index"},
        {"name":"layerTrkMC",   "type":"L",  "info":"bitmask of tracking detector layers with true hits by the simulated particle"},
        {"name":"layerNeuMC",   "type":"L",  "info":"bitmask of neutral detector layers with true hits by the simulated particle"},
        {"name":"layerTrkRec",  "type":"L",  "info":"bitmask of tracking detector layers used by the reconstructed particle"},
        {"name":"layerNeuRec",  "type":"L",  "info":"bitmask of neutral detector layers used by the reconstructed particle"}
        ]
    }
rafopar commented 1 year ago

The one in the development is outdated, On the branch iss540_iss731, it has the right definition of banks. Here is the current definition of the matching banks https://github.com/JeffersonLab/clas12-offline-software/blob/iss540_iss731/etc/bankdefs/hipo4/mc.json#L104-L133

As the TruthMatching is not fully functional yet, it probably can be removed from the development branch for pass2. Let me know, if you want the current bank definitions in iss540_iss731 to be modified as well.

baltzell commented 1 year ago

Even if the matching is not fully functional, I think if the bank format can be finalized and in the pass2 release (and pull in your latest functionality, and delete the old branch) that would be worthwhile.

Regarding the "tid", I would definitely keep it the exact same variable name as what it links to in MC::Particle, which sounds like is "tid".

baltzell commented 1 year ago

Or, looks like you switch to row index instead of tid, which is also probably fine. Maybe just put in a PR?

rafopar commented 1 year ago

Ok @baltzell , then if I understood you correctly, I will create a pull request without changing anything.

baltzell commented 1 year ago

I gave it a look, and that branch is so old that is has a lot merge/rebase conflicts with development. I think the easiest and best thing to do would be to make a new branch off development and manually pull in the changes to the small number of files relevant to MC matching. Looks like it should only be a couple JSON files and a couple Java files?

rafopar commented 1 year ago

Ok, I have some questions/clarifications:

There are three files that are different between the development and the iss540_iss731,

-- I know that TruthMatch.java should be completely replaced by the one in iss540_iss731, -- In mc.json there is a bank MC::User which has the same "item" : 5, as the bank "MC::GenMatch", I don't remember if "MC::User" is still being used? somehow I though it was decided to not use it, but if it will be used, then I can shift Truth Match bank "item"s by one. -- About EvioDataEventHandler.java, as gemc produces hipo files directly, in the new branch I will not touch this file, and will change only MC::User and TruthMatch.java

If you agree with this, I can do those changes.

baltzell commented 1 year ago

Yes, I'd propose to completely discard the changes to EvioDataEventHandler.java and make a new branch with just the JSON and TruthMatch.java changes.

rafopar commented 1 year ago

Ok, pushed the iss1081_TruthMatchPR to remote. if it passes the checks, will create a PR.

baltzell commented 1 year ago

closed by #1085