Fix ADC Nbit = 14 and elecGain = 7.8 in wirecell, new correct values#134
Fix ADC Nbit = 14 and elecGain = 7.8 in wirecell, new correct values#134aolivier23 merged 25 commits intodevelopfrom
Conversation
emanuele-villa
requested review from
absolution1,
brettviren and
wenqiang-gu
|
|
||
| // The resolution (bits) of the ADC | ||
| resolution: 12, | ||
| resolution: 14, // used to be 12 bits |
There was a problem hiding this comment.
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.
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.
I will use extVar there in any case, ready for whenever that file will start being used
There was a problem hiding this comment.
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.
|
I changed all jsonnet files to read the resolution from fcl, because I'd leave the reading from params.jsonnet for the refactoring (it's pretty deep and I think also include paths have to be fixed a bit). I was unfortunately not able to test, we can talk about it in the call in a couple minutes. |
|
You can see in the last commits that I've done what we discussed: read the Nbit and elecGain values from a standard value defined in dunecore (DUNE/dunecore#141), removed all references to pgrapher/ paths, reading resolution from common/params.jsonnet. I expect there to be mistakes but I have not had time to test how jsonnet dumps the configurations now and check if they remained consistent. @HaiwangYu if you have time to do it, it's excellent. Otherwise, I can try to find time tomorrow so that it can go in this week's release. |
This reverts commit 166cc82. Haiwang has asked to keep the pgrapher paths for now and change them in a second moment.
This reverts commit f51db30. Useless, reverting also the commit that added these lines
…jsonnets" This reverts commit b939343. Handling reading from fcl only from dune/params.jsonnet.
|
I have performed checks using the jsonnet command to generate the json configurations and the value of |
|
trigger build |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
parent CI build details are available through the CI dashboard |
|
❌ CI build for DUNE Failed at phase ci_tests DUNE on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests DUNE phase logs parent CI build details are available through the CI dashboard |
|
I like the idea of keeping the live DUNE configurations for WireCell in DUNE repositories. That way we can make changes on our own schedule and not have to wait for PRs to be approved externally, and also requiring a LArSoft release. These latter come frequently enough anyway, but there may be a future in which changing a DUNE parameter in a release will be much quicker if it's only DUNE that has to cut the release. |
|
Hi @emanuele-villa thanks a lot for making this major refactoring! As we discussed offline, I am happy with the current status. Thanks! |
|
Hello @aolivier23 I think the required approvals went through. Is the failed test above something that needs fixing? |
|
trigger build |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
parent CI build details are available through the CI dashboard |
|
❌ CI build for DUNE Failed at phase ci_tests DUNE on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests DUNE phase logs parent CI build details are available through the CI dashboard |
|
@emanuele-villa there's something wrong with one of your new .fcls. I'll try to fix it before I cut the release today. Please fix it if you get to it before I do. 1: Failed to parse the configuration file 'ci_test_datareco_protoDUNEsp.fcl' with exception |
|
This seems to work locally, and I can't figure out what would be wrong with the .fcl file chain. Maybe it just relied on another PR that I merged later? I'll merge it then deal with the consequences. Thank you for your patience. |
|
@emanuele-villa I had to revert this because it breaks reco tests for ProtoDUNE-SP and others. You can see some examples here: https://dbweb8.fnal.gov:8443/LarCI/app/ns:dune/build_detail/test_details?build_id=dune_ci/19122&platform=Linux%20slf7&buildtype=slf7%20e26:debug&phase=ci_tests&test=ci_datareco_regression_test_protoDUNEsp Please create a new PR using the branch from this PR, fix the problems in the CI tests, then I'll merge the new PR this week. Sorry about the mess. |
|
Ok this one I think I understand, by reverting some commits some lines of the fcl disappeared and I did not notice. |
7 participants
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
NbitandelecGainfrom fcl in allparams.jsonnetfiles.The values in
wirecell_dune.fclshould 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.