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

Fix ADC Nbit = 14 and elecGain = 7.8 in wirecell, new correct values #134

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

emanuele-villa
Copy link
Member

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 and elecGain from fcl in all params.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.

@@ -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
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

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

Copy link
Member

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.

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.

3 participants