Skip to content

CRV changes#1334

Merged
brownd1978 merged 6 commits intoMu2e:mainfrom
ehrlich-uva:WidebandSimReco2
Oct 15, 2024
Merged

CRV changes#1334
brownd1978 merged 6 commits intoMu2e:mainfrom
ehrlich-uva:WidebandSimReco2

Conversation

@ehrlich-uva
Copy link
Copy Markdown
Contributor

@ehrlich-uva ehrlich-uva commented Sep 8, 2024

lowered the threshold for the "big cluster" option in the CRV coincidence finder to speed up the process for events that have larger numbers of hits, changed CRV digitization/timing to agree with the new FEB firmware, removed temporary implementation for CRV in ArtFragment reader

…ence finder, changed CRV digitization end, removed temporary implementation for CRV in ArtFragment reader, added double-sided readouts for CRV Wideband simulation
@FNALbuild
Copy link
Copy Markdown
Collaborator

Hi @ehrlich-uva,
You have proposed changes to files in these packages:

  • CRVConditions
  • CRVReco
  • CRVResponse
  • DAQ
  • Mu2eG4

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

The following users requested to be notified about changes to these packages:
@resnegfk

⌛ The following tests have been triggered for f8375da: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at f8375da.

Test Result Details
test with Command did not list any other PRs to include
merge Merged f8375da at fa344e3
build (prof) Log file. Build time: 04 min 12 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
FIXME, TODO 🔶 TODO (1) FIXME (3) in 3 files
clang-tidy 🔶 6 errors 42 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at f8375da after being merged into the base branch at fa344e3.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@kutschke
Copy link
Copy Markdown
Contributor

kutschke commented Sep 8, 2024

How will this PR interact with existing simulated datasets, both onspill and extracted position?

@ehrlich-uva
Copy link
Copy Markdown
Contributor Author

@kutschke

  • Lowering the threshold for "large clusters" for the coincidence finder will make the coincidence finder run faster for events that have a huge number of CRV hits. For these rare events, the number and properties of the coincidence clusters will change.
  • The change of the digitization end will reduce the number of CRV hits (onspill), because up until now, the digitization period went into the beam flash (55ns into the new event). This change is required to agree with the new FEB firmware.
  • The removal of the temporary implementation of a part of the CRV code in the ArtFragment reader has no impact on the data.
  • The new double-sided readout for the Wideband simulation is a new Wideband geometry, which has no impact on the "regular" Mu2e simulations.

@kutschke
Copy link
Copy Markdown
Contributor

kutschke commented Sep 9, 2024

Thanks Ralf.

  • Lowering the threshold for "large clusters" for the coincidence finder will make the coincidence finder run faster for events that have a huge number of CRV hits. For these rare events, the number and properties of the coincidence clusters will change.

As I understand it we can still get correct results using the existing datasets. The next time we remake the reco datasets the output will change but it will be an improvement.

  • The change of the digitization end will reduce the number of CRV hits (onspill), because up until now, the digitization period went into the beam flash (55ns into the new event). This change is required to agree with the new FEB firmware.

Ok. So again we can sill use the existing datasets with the understanding that this detail is wrong. The next time we remake digis, the simultated events will improve.

Do I understand both correctly?

Is there anyone who can meaningfully review the intellectual content of the PR. I will look through it for technical compliance.

@kutschke kutschke self-assigned this Sep 9, 2024
@ehrlich-uva
Copy link
Copy Markdown
Contributor Author

Do I understand both correctly?

Yes.

Is there anyone who can meaningfully review the intellectual content of the PR. I will look through it for technical compliance.

I think @oksuzian would be best to review the change.

@kutschke kutschke requested a review from oksuzian September 9, 2024 18:04
Copy link
Copy Markdown
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the large combinatoric problem. I have some questions about the timing change, and request that all the timings be consolidated.

Comment thread CRVResponse/fcl/prolog_v11.fcl Outdated
//note: if measured deviations are used, the cutoffs should be set to the maximum values
digitizationStart : 400.0 //400ns
digitizationEnd : 1750.0 //1750ns
digitizationEnd : 1695.0 //1695ns
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this a hardware number? or is it configurable in the CRV DAQ?
Because of the mismatch in timing between booster and extraction frequencies some events will have length 1705. Does this mean the CRV will be blind to those last 10ns?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Following up on Dave's comment. Even with the existing simulations, approximately every 5th event is 25 ns shorter than the other 4. There is a data product that gives the duration of the current event. Does your digitization code use this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need to find out more details about how the timing will be handled in the CRV's FEB firmware. Currently, I use the event window length (from EventWindowMarker::eventLength()) only for Offspill. I will change the code for Onspill depending on what answer I get about the firmware.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What is the range of event lengths we can expect for Onspill? Do you have a doc-db number where it is explained?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Look at page 8 of Mu2e-doc-19095 .

The bottom line is that onspill events will either by 67 or 68 ticks of the 25 ns DAQ clock, so either 1675 or 1700 ns.
It's Ok to digitize beyond 1700 ns so long as there all code properly understands start times relative to the beam arrival. Averaged over a long time there will be approximately 4 events of 1700 ns for every one event of 1675 ns, averaging to 1695 ns. But true period of the delivery ring is not exactly 1695 ns - I forget if it's a little longer or a little shorter. So the pattern will be approximately a repeating pattern of 4+1 but with some 3+1 or 5+1 mixed in.

This implies that the proton arrival time at the production target moves around within the stable 25 ns clock.

In the present simulation scheme we have:

https://github.com/Mu2e/Offline/blob/main/DataProducts/inc/EventWindowMarker.hh

  • This has spill type (on or off spill) and event duration, either 1675 or 1701

https://github.com/Mu2e/Offline/blob/main/MCDataProducts/inc/ProtonBunchTimeMC.hh

  • MC truth about the proton arrival time at the production target relative to the DAQ t0

https://github.com/Mu2e/Offline/blob/main/RecoDataProducts/inc/ProtonBunchTime.hh

  • Reco value of the proton arrival time at the production target

Richie is the expert on what's in the code now. See his talk at Mu2e-doc-39805.

Richie's model captures the end result well but it will need an evolution to capture what we will really get from the DAQ.

My draft PR #1252 is the first step toward moving us closer to what the actual data will look like. I will send an email to the people on this PR that points to a note in progress that discusses more. I don't want to post the link in the clear.

For now, your code should match Richie's model.

Let me know if you have any questions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bonventre - FYI I mentioned you as the expert in the current model of event timing in a reply to Ralf on this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bonventre - FYI I mentioned you as the expert in the current model of event timing in a reply to Ralf on this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The model as I've implemented is that in OnSpill the digitization window has a fixed width that must be less than 1675 ns (minimum event length), but is delayed by a fixed time after the arrival of the event window start marker from the DAQ so it can extend past 1695 ns. The CRV firmware hopefully is implementing something similar. I also only use the eventLength in OffSpill to determine the digitization end time

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The CRV code has been changed to the following: The on-spill digitization starts at 400ns after the event window start, where the event window starts at the first clock tick after protons (between 0ns and 25ns after protons). The on-spill digitization ends at the end of the event window, where the event window length is either 1675ns or 1700ns.

Comment thread CRVResponse/fcl/prolog_v11.fcl Outdated
crvSiPMChargesModuleLabel : "CrvSiPMCharges"
digitizationStart : 400.0 //400ns
digitizationEnd : 1750.0 //1750ns
digitizationEnd : 1695.0 //1695ns
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please consolidate all these numbers to a single fcl block in prolog to avoid replication.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like you have units in the comments. Please don't repeat the numerical values in the comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@kutschke kutschke requested a review from rlcee September 10, 2024 01:21
Comment thread CRVResponse/fcl/prolog_v11.fcl Outdated
//note: if measured deviations are used, the cutoffs should be set to the maximum values
digitizationStart : 400.0 //400ns
digitizationEnd : 1750.0 //1750ns
digitizationEnd : 1695.0 //1695ns
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Following up on Dave's comment. Even with the existing simulations, approximately every 5th event is 25 ns shorter than the other 4. There is a data product that gives the duration of the current event. Does your digitization code use this?

Comment thread CRVResponse/fcl/prolog_v11.fcl Outdated
crvSiPMChargesModuleLabel : "CrvSiPMCharges"
digitizationStart : 400.0 //400ns
digitizationEnd : 1750.0 //1750ns
digitizationEnd : 1695.0 //1695ns
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like you have units in the comments. Please don't repeat the numerical values in the comments.

@kutschke
Copy link
Copy Markdown
Contributor

@rlcee I added you as a review to discuss with Ralf about one file:

 CRVConditions/data/wideband4modules2sides.txt

Is this OK as a file or should it be in the Conditions DB?

@ehrlich-uva
Copy link
Copy Markdown
Contributor Author

@rlcee I added you as a review to discuss with Ralf about one file:

 CRVConditions/data/wideband4modules2sides.txt

Is this OK as a file or should it be in the Conditions DB?

We already have the CRVConditions/data/wideband4modules.txt and CRVConditions/data/wideband1module.txt as txt files and we may get more of them. They are special setups used by only three people. In a discussion we had a while ago, we decided that it was Ok to have them as txt files.

@rlcee
Copy link
Copy Markdown
Collaborator

rlcee commented Sep 10, 2024

@rlcee I added you as a review to discuss with Ralf about one file:

 CRVConditions/data/wideband4modules2sides.txt

Is this OK as a file or should it be in the Conditions DB?

We already have the CRVConditions/data/wideband4modules.txt and CRVConditions/data/wideband1module.txt as txt files and we may get more of them. They are special setups used by only three people. In a discussion we had a while ago, we decided that it was Ok to have them as txt files.

The critical point is that no data or sim production should use these files. I'd prefer not to commit them at all, but as Ralf says, we agreed to allow them for personal use.

@FNALbuild
Copy link
Copy Markdown
Collaborator

📝 The HEAD of main has changed to e389271. Tests are now out of date.

@ehrlich-uva
Copy link
Copy Markdown
Contributor Author

I made the changes to address the recommendations/requests.

@ehrlich-uva
Copy link
Copy Markdown
Contributor Author

@kutschke I made the requested changes.

Copy link
Copy Markdown
Contributor

@kutschke kutschke left a comment

Choose a reason for hiding this comment

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

Approving from my phone on a plane!

@brownd1978 brownd1978 merged commit 57339a5 into Mu2e:main Oct 15, 2024
@brownd1978
Copy link
Copy Markdown
Collaborator

@FNALbuild run build test

@kutschke
Copy link
Copy Markdown
Contributor

The final CI did not run because Dave merged 1 minute before I ran it.

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.

7 participants