-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix ADC Nbit = 14 and elecGain = 7.8 in wirecell, new correct values #134
base: develop
Are you sure you want to change the base?
Conversation
@@ -107,7 +107,7 @@ local wc = import "wirecell.jsonnet"; | |||
baselines: [900*wc.millivolt,900*wc.millivolt,200*wc.millivolt], | |||
|
|||
// The resolution (bits) of the ADC | |||
resolution: 12, | |||
resolution: 14, // used to be 12 bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, you can make this the only site that uses std.extVar("NBits")
and then leave all the detector-specific params.jsonnet
files to inherit this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this I think I'd need to update all the includes from local base = import "pgrapher/common/params.jsonnet";
to local base = import "common/params.jsonnet";
(or the right dunereco path).
I thought it was better to remove all wire-cell-toolkit paths together in the restructuring rather than a piece now and the rest later. Do you think it is better to change the path of params.jsonnet
now already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use extVar there in any case, ready for whenever that file will start being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this I think I'd need to update all the includes from local base = import "pgrapher/common/params.jsonnet"; to local base = import "common/params.jsonnet"; (or the right dunereco path).
Yes, exactly. I wrongly assumed that was already the case. Otherwise, a DUNE-specific common/params.jsonnet
is vestigial.
In fact, let me step back and suggest to copy all the rest of WCT's pgrapher/common/
as well so it lands at dunereco/DUNEWireCell/common/
. Then, any occurrence of import "pgrapher/common/<whatever>
must be changed to be import "common/<whatever>
.
The main motivation to put (almost) all Jsonnets in one DUNE tree is that it helps the human to have "everything" in one spot. OTOH, if DUNE keeps a dependency on WCT's pgrapher/common/
for some files then that human must search WIRECELL_PATH
to find the dependencies. Not super hard, I admit, but it is just one more thing to mentally carry.
Secondarily, it allows DUNE's copy to deviate from WCT's in the future. WCT's common/
area has to be kept broadly generic in order to support all detectors. DUNE can, in principle, simplify its configuration once free from this requirement.
This is a new MR since the other one got accidentally merged (and then reverted) as it was not marked as draft.
See #131 for more context.
Following up on the discussion in #132, I added the reading of
Nbit
andelecGain
from fcl in allparams.jsonnet
files.The values in
wirecell_dune.fcl
should be correct for all detectors; I left untouched the protodunesp NBit value and the elecGain for all protodune and coldbox configs (since the value changed only in the middle of July). I would recommend to access the gain value through database, in some way.I have not yet tested if the jsons are created correctly with
wcsonnet
, I'll try to do that between today and tomorrow (need to set up some script or I'll get crazy).If you prefer to hold this until our meeting, it's ok.