-
Notifications
You must be signed in to change notification settings - Fork 19
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
Change adc range from 12 to 14 bits, new DAQ standard #88
Conversation
After this commit, this MR shall go in develop after DUNE/dunecore#141 |
By testing I realised I was making a mistake: can't include in a tools fcl. Therefore, I just updated the Nbit value with a comment. |
A question -- does this affect PDSP MC? I think we simulated some new MC recently for PDSP. |
Added Jake and Tingjun as reviewers. |
Looks like dunecore PR 141 adds legacy support. |
I left the value of Nbit = 12 in all pdsp fcl configuration in wirecell_dune.sh. Given that now Nbit (aka resolution) is always read from fcl, there should be no problem. |
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.
Approved
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 |
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 |
This PR fails a regression test. You can see an example here: @tomjunk do you know what that regression test is testing for? Can we get around it somehow? |
🚨 For more details about the warning phase, check the ci_tests DUNE phase logs parent CI build details are available through the CI dashboard |
🚨 For more details about the warning phase, check the ci_tests DUNE phase logs parent CI build details are available through the CI dashboard |
parent CI build details are available through the CI dashboard |
Realized that this PR depends on 1 or more other PRs. I'll merge them all then try again and deal with the consequences. |
@emanuele-villa I had to revert this because it breaks a regression test. Looks like an easy fix with your knowledge of how the ADCs work. You have to create a new PR with the branch from this PR and go back through the PR process. Sorry for the mess, I merged this hoping the new Wirecell version would let me fix some problems. |
I suspect it could be because there is a comment after the value Nbit: 14... |
this PR in the end does not depend on any other, so it should not be the problem |
@@ -59,7 +59,7 @@ int test_IdealAdcSimulator(bool useExistingFcl) { | |||
fout << "mytool: {" << endl; | |||
fout << " tool_type: IdealAdcSimulator" << endl; | |||
fout << " Vsen: 2.0" << endl; | |||
fout << " Nbit: 12" << endl; | |||
fout << " Nbit: 14" << endl; |
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 think the problematic line with the unit test is here.
This shall go in only after changes in dunereco/DUNEWirecell, where we're moving from 12 to 14 as well.
Note that the DAQ standard has been 14 for a long time now, but the value was never updated.
(duplicate of another MR that was reverted, this shall remain a draft for a while)