Skip to content

Update Nbit (2nd attempt)#90

Draft
emanuele-villa wants to merge 1 commit intodevelopfrom
fix/adc_nbit
Draft

Update Nbit (2nd attempt)#90
emanuele-villa wants to merge 1 commit intodevelopfrom
fix/adc_nbit

Conversation

@emanuele-villa
Copy link
Member

This is the same as #89, which was reverted due to CI failing.

I just removed the comment that I believe might cause CI to fail (the check sees more than the numerical value for that field). If it's something else, we can push more commits.

@tomjunk
Copy link
Member

tomjunk commented Mar 24, 2025

Hi Emanuele,

I think I see what went wrong with the unit test: test_idealAdcSimulator.cxx test:

3710f7f#diff-9d8d85b658b1e80645f1cf4f58f27615392ab4d6cb1408c5eb6d921fd6505463L62

The test in question is a pretty well self-contained unit test, i.e. it actually creates its own fcl file and does not include any others. The code in the test assumes a 12-bit ADC, which cannot read out a number bigger than 4095. This is all fine, and probably should stay the way it is, because we do in fact still want to simulate 12-bit ADCs (though I don't know if we still use the Idea ADC simulator tool anymore). So to change the unit test's Nbits to 14 without making changes to the expected outcomes on signals that saturate a 12-bit ADC causes the test to fail. Changing comments in fcl files is unlikely to have an impact.

That said, PR 89 is the reversion of PR 88, and this PR so far contains just one commit and does not re-do 88. Andrew mentioned that this test failure was just one of two problems -- there was also a WireCell configuration error he ran into.

Converting to draft, just in case. PRs that are not ready to merge should be drafts.

@tomjunk tomjunk marked this pull request as draft March 24, 2025 19:44
@emanuele-villa
Copy link
Member Author

I see, thanks for looking into it.

When I had a discussion with some people including @absolution1 it was said to change the Nbit also here, so that in case people are still using the 1D simulation, they at least have the correct ADC range. If it's not needed and it's easier to not touch it, I am happy to just forget about it. The Wirecell changes are independent of this.

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