changed CRV gap size for extracted position #1226
Conversation
|
Hi @ehrlich-uva,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. The following users requested to be notified about changes to these packages: ⌛ The following tests have been triggered for 2d018be: build (Build queue is empty) |
|
☀️ The build tests passed at 2d018be.
N.B. These results were obtained from a build of this Pull Request at 2d018be after being merged into the base branch at c5b77ad. For more information, please check the job page here. |
Just to make sure I understand: the intention is to hold off on the merge until we define an MDC2020 legacy branch in Offline. Then merge this into the branch that will be used for MDC2024. Is that correct? |
|
On Wed, Mar 27, 2024 at 7:36 AM Rob Kutschke ***@***.***> wrote:
Should be merged after all changes w.r.t. the redigitization of MDC2020
are done.
Just to make sure I understand: the intention is to hold off on the merge
until we define an MDC2020 legacy branch in Offline. Then merge this into
the branch that will be used for MDC2024. Is that correct?
I'd like to avoid branching until absolutely necessary. If we can prove
that the gap change is compatible with existing MDC2020 datasets then it
can merged before branching. If not, we branch. But it's not required for
the more important task of getting the DB setup for the dig reprocessing so
we can move forwards quickly.
Dave
… —
Reply to this email directly, view it on GitHub
<#1226 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAH576OYQYX5YE6KNGTJ7DY2LKNPAVCNFSM6AAAAABFJNASRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRSHEZTIOBSHA>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
--
David Brown ***@***.***
Office Phone (510) 486-7261
Lawrence Berkeley National Lab
M/S 50R5008 (50-6026C) Berkeley, CA 94720
|
|
📝 The HEAD of |
|
@ehrlich-uva this is a low priority comment since this PR is waiting for the first round of MDC2024 commits. When you get a chance please followup on the one comment, to remove historical comments. Thanks. |
|
Lets clear out the current list of other PRs to Offline, except 1252, then declare MDC202o to be closed. |
and then merge this one after a round of tags of final MDC2020 tags. That works for me. When you are ready sign off on Giani's 3 PRs and will look after the final CI and merges. |
|
@kutschke your review still 'requests changes', can you please update that? |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 2d018be: build (Build queue is empty) |
|
☀️ The build tests passed at 2d018be.
N.B. These results were obtained from a build of this Pull Request at 2d018be after being merged into the base branch at 3661cc3. For more information, please check the job page here. |
@ehrlich-uva Ralf: my one comment was to ask you to remove the comments with historical values. Comments to say where the current values come from are good. My reason is that comments of this type create multiple points of maintenance that are often missed - so after the next edit the comments become misleading. We can always find historical values with git log and git diff. |
|
@ehrlich-uva The plan has changed. Since this only affects the extracted position Dave has decided to merge into main rather than waiting for a new branch for breaking changes. Please make the requested changes to the comments and I will approve and merge. |
|
📝 The HEAD of |
| @@ -16,8 +16,8 @@ double crs.scintillatorBarLengthT2 = 3200; //2 CRV-L endcap mod | |||
| double crs.scintillatorBarThickness = 19.8; //mm | |||
| double crs.scintillatorBarWidth = 51.3; //mm | |||
| double crs.layerOffset = 42; //mm | |||
| double crs.gapLarge = 0.5; //mm | |||
| double crs.gapSmall = 0.0875; //mm | |||
| double crs.gapLarge = 0.2; //mm //0.366 according to crv_parameters.xlsx | |||
There was a problem hiding this comment.
What's going on here - the value of the parameter is different than the value given in the reference?
This confuses me.
There was a problem hiding this comment.
Our Wideband studies found that a gap of 0.2mm matches our data better than the previously estimated 0.366mm (which are also used in the reference). That's why we want to go forward with 0.2mm for the KPP simulations. We are continuing our Wideband studies, but we think that a 0.2mm effective gap is the best value to use. The main source of the disagreement is the non-uniform thickness of the TiO2 coating of the CRV counters. We will eventually make new CRV lookup tables that will take our findings into consideration.
There was a problem hiding this comment.
Is there a reference that discusses this? If so, update the comment to point to it. If not update the comment to say that this an empirically determined effective value or something like that. For long comments, my preferred style is to put numbered notes at the top of the file and for the inline comment say "See note 1)". This allows you to write as much as you need to tell the story well without breaking up the visually appearance at the point of the comment.
There was a problem hiding this comment.
Is a change actually requested here? Ralf answered your question. Should we block this PR because of a comment?
|
Ralf, can you confirm that these changes don't affect CRV reconstruction? If so, I think we can merge them. |
|
Hi Dave, it affects the simulation and reconstruction of the extracted position, but not the regular CRV. |
|
Lets talk to Yuri. If we can run a new production soon we can merge this.
There are already cases where newer code is incompatible with older data
sets, so this will just be one more case to list in that table.
…On Fri, Sep 13, 2024 at 5:42 AM ehrlich-uva ***@***.***> wrote:
Hi Dave, it affects the simulation and reconstruction of the extracted
position, but not the regular CRV.
—
Reply to this email directly, view it on GitHub
<#1226 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAH575Y63UOYHZF4NR53ZDZWLMUNAVCNFSM6AAAAABFJNASRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBYHA3DSMJVGU>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
--
David Brown ***@***.***
Office Phone (510) 486-7261
Lawrence Berkeley National Lab
M/S 50R5008 (50-6026C) Berkeley, CA 94720
|
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 3a0399e: build (Build queue is empty) |
|
☀️ The build tests passed at 3a0399e.
N.B. These results were obtained from a build of this Pull Request at 3a0399e after being merged into the base branch at 305a8d6. For more information, please check the job page here. |
|
📝 The HEAD of |
| @@ -16,8 +16,8 @@ double crs.scintillatorBarLengthT2 = 3200; //2 CRV-L endcap mod | |||
| double crs.scintillatorBarThickness = 19.8; //mm | |||
| double crs.scintillatorBarWidth = 51.3; //mm | |||
| double crs.layerOffset = 42; //mm | |||
| double crs.gapLarge = 0.5; //mm | |||
| double crs.gapSmall = 0.0875; //mm | |||
| double crs.gapLarge = 0.2; //mm //0.366 according to crv_parameters.xlsx | |||
There was a problem hiding this comment.
Is there a reference that discusses this? If so, update the comment to point to it. If not update the comment to say that this an empirically determined effective value or something like that. For long comments, my preferred style is to put numbered notes at the top of the file and for the inline comment say "See note 1)". This allows you to write as much as you need to tell the story well without breaking up the visually appearance at the point of the comment.
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for a484929: build (Build queue is empty) |
|
☀️ The build tests passed at a484929.
N.B. These results were obtained from a build of this Pull Request at a484929 after being merged into the base branch at 95d61ac. For more information, please check the job page here. |
Should be merged after all changes w.r.t. the redigitization of MDC2020 are done.