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

PTB decoder module and data product #397

Merged
merged 9 commits into from
Feb 16, 2024

Conversation

tomjunk
Copy link
Contributor

@tomjunk tomjunk commented Nov 21, 2023

This decoder is based on the ProtoDUNE-SP one, but has fixes and enhancements. It separates out the HLTs from
the LLTs, and provides structs for CHStatus, Feedback, and Misc. The WordIndex struct is there to keep track of
the ordering of words in the input fragments after they have been divided up by type. People on ProtoDUNE-SP
requested a function that provides the CHStatus word that comes immediately before any given HLT. This function
is provided in PDSPCTBRawUtils.cxx.

@tomjunk tomjunk assigned tomjunk and unassigned tomjunk Nov 21, 2023
@tomjunk
Copy link
Contributor Author

tomjunk commented Nov 21, 2023

I'd suggest Tereza Kroupova as a reviewer. I also have a gallery example of displaying the contents of the sbndptb data product if it can be added somewhere. The gallery test is a little hacky in that PDSPPTBRawUtils.cxx has to be included in it.

change process name to PTBDECODER (was: TPCDECODER) -- just cosmetic
@tomjunk
Copy link
Contributor Author

tomjunk commented Dec 5, 2023

Looks like the sbndcode/Decoders/CMakeLists.txt file now has two new additions -- one for SPECTDC and one for PTB. I think that's the only clash. Shall I re-synch my branch?

@tomjunk
Copy link
Contributor Author

tomjunk commented Jan 8, 2024

See SBNSoftware/sbndaq-artdaq-core#93 for some plots of a test of this decoder in run 9660.


struct Trigger {
ULong64_t timestamp;
ULong64_t trigger_word;

Choose a reason for hiding this comment

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

I believe this doesn't format the trigger word correctly as the word itself is only 61 bits and the first 3 bits are the trigger type. In this case I think it's concatenated and then I am not sure how the trigger type is determined. Is there a way how we can just use the same structs in PTB_content.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is an offline data product, which is not a bit-for-bit copy of the contents of the fragment, but it is meant to carry all of the bits from the PTB, just in an easier-to-digest format. So it is "fluffier" -- not packed so tightly. The reason for this is to allow no-code solutions like using TTree::Scan() and TTree::Draw to see the trigger word and the word type separately without having to do bit manipulation (which is also possible with TTree::Scan() and TTree::Draw()). I think the packing in the PTB was just done to fit it all in the size of the words that come up, and 3 bits is too small to justify another word, so it was packed in. We don't have that constraint offline, so naming all the pieces is more convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same pattern is used in the other word types, like ChStatus. The idea is not to have to rely on getters when browsing data and debugging things -- ROOT's Tree scanners and drawers have some limitations. I found that the method that gets the last ChStatus before a HLT was too complicated for ROOT's serializer to deal with, so I broke it out of the header and into its own piece of code. Simple getters like the ones in PTB_content.h might work, but even simpler to have named fields in the structs.

Copy link

@terezakroupa terezakroupa Jan 17, 2024

Choose a reason for hiding this comment

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

Hi Tom, but the CTB fragment does have these defined separately too, right? My concern is that the bits are not mapped correctly based on what I've seen from Lynn using this decoder. It seems (for example) like the trigger word that it spits out is 64bits and includes the trigger type (i.e. it's not just padded with zeros). Maybe you can explain to be better what is happening over zoom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Tereza -- oh, could it be that Lynn is not using the updated PTB_content.h in sbndaq-artdaq-core/Overlays/SBND/PTB_content.h? The number of bits in the trigger word in the fragment was wrong before that PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi very late follow up to this; entirely possible that I was using an outdated PTB_content.h file! I didn't have the sbndaq-artdaq-core repo pulled down locally, so was using whatever version was available by default on cvfms (ups list gives me v1_08_00of1).

@fjnicolas
Copy link
Contributor

@tomjunk @terezakroupa what is the status of this PR? is it ready to go or still needs more work? Thanks!

@tomjunk
Copy link
Contributor Author

tomjunk commented Jan 29, 2024

Hi Fran,

Still waiting for Tereza or Gabriela to reply. The bits that come up from the PTB need to match what's in sbndaq-artdaq-core/Overlays/SBND/PTB_content.h, and that's a separate PR in the sbndaq-artdaq-core repo's offline branch. This PR can go in before or after that one, but to get everything to work properly, both should go in and sbndcode built with the new sbndaq-artdaq-core. Looks like that PR for the offline branch was merged six hours ago.

SBNSoftware/sbndaq-artdaq-core#93

@tomjunk
Copy link
Contributor Author

tomjunk commented Jan 29, 2024

If you'd like me to test it, I have a working build on my desktop (though the wired network in my office is broken today; service desk ticket is being worked on).

@tomjunk
Copy link
Contributor Author

tomjunk commented Jan 29, 2024

I tested this PR on an older commissioning data file but I am happy to do more tests with data with more complete PTB info.

@tomjunk
Copy link
Contributor Author

tomjunk commented Feb 6, 2024

I did run it as part of the PDS decoder test. It didn't crash, but it might be nice to know what to expect from some of this commissioning data.

@tomjunk
Copy link
Contributor Author

tomjunk commented Feb 15, 2024

Lynn has been running it. Can we get this in?

@lynnt20
Copy link
Contributor

lynnt20 commented Feb 15, 2024

trigger build

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

❌ CI build for SBND Failed at phase unit_test SBND on slf7 for c14:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the unit_test SBND phase logs

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link
Collaborator

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@lynnt20
Copy link
Contributor

lynnt20 commented Feb 15, 2024

Investigating the WireCell simpleSC label errors in the CI tests...

Fran is right and I mixed up the CI tests between different PRs, my bad! 😅

@fjnicolas
Copy link
Contributor

@lynnt20 the c14 error is expected (#407) and the warnings with e26 look unrrelated to this PR, so it should be good to go!

@fjnicolas fjnicolas merged commit 7ab47db into SBNSoftware:develop Feb 16, 2024
3 of 4 checks passed
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.

5 participants