Add infrastructure and initial set of tests for ADQ component #215
Conversation
|
Also contains a couple of fixes that came up when writing the tests... |
philsmt
added
the
skip-changelog
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #215 +/- ##
==========================================
+ Coverage 71.09% 75.87% +4.77%
==========================================
Files 23 23
Lines 2726 2727 +1
==========================================
+ Hits 1938 2069 +131
+ Misses 788 658 -130 ☔ View full report in Codecov by Sentry. |
tests/mockdata/adq.py
Outdated
| for i, ch_label in enumerate(self.channel_labels): | ||
| # Add a channel-dependent baseline shift and gaussian to | ||
| # each channel. | ||
| root_grp[f'channel_{ch_label}/raw/samples'][:] += -10 * (i+1) \ | ||
| + gaussian(x, 0, i*80, i*1000, 50).astype(np.int16) |
There was a problem hiding this comment.
Would it make sense to do this optionally for a selected single channel? We don't generally fill mock files with random data, and I imagine this is going to make the tests slower & more memory intensive, especially on systems using tmpfs (i.e. temporary files are created in RAM).
There was a problem hiding this comment.
Sure, it shouldn't be a problem to limit actual data to a few select channels. But unlike with EXtra-data, I'd consider real data in the files essential to test what the components do - in particular if they contain transformations.
There was a problem hiding this comment.
87a9fb3 limits data generation to channels selected in the fixture, and I reduced the trace length to 50000 (which should be enough for any realistic data-based tests). That should keep the memory cost negligible while the fixture is only generated once per session.
philsmt
force-pushed
the
feat/adq-tests
branch
from
b870637
to
362b1a2
Compare
philsmt
force-pushed
the
feat/adq-tests
branch
from
362b1a2
to
87a9fb3
Compare
| @@ -124,7 +124,7 @@ def __init__(self, data, channel, digitizer=None, pulses=None, | |||
| raise ValueError('channel expected to be 2 or 3 characters, ' | |||
| 'e.g. 1A or 1_A') | |||
|
|
|||
| self._channel_number = ord(self._channel_letter) - ord('A') | |||
| self._channel_number = ord(self._channel_letter) - ord('A') + 1 | |||
There was a problem hiding this comment.
Are we confident no-one is already using channel numbers as they are now, counting from zero?
There was a problem hiding this comment.
Yes, the numbers and letters are enumated at initialization time by the AdqDigitizer device and cannot be configured. They're not even (always) in the physical order in the crate.
There was a problem hiding this comment.
Sorry, perhaps I'm misunderstanding. At the moment, if I create adq = AdqDigitizer(run, '1C'), then adq.number will give me 2 (A=0, B=1, C=2), yes? With this change, it becomes 3. Are you confident that no-one has started using those numbers as they are?
(We can still change it even if someone might be using it - I just want to think about if we need to announce the change somehow)
There was a problem hiding this comment.
Apologies, I understand now. So the earlier behaviour was a bug, adq.number was always meant to represent the board number in the sense it was used in CONTROL parameters (yes, someone funny used letters for fast data and numbers for slow data). As a matter of fact, .channel_parameters would've given you the wrong values as of now.
I highly doubt this parameter was used already directly, but if you feel strongly about it I can add it as a bugfix.
There was a problem hiding this comment.
OK, thanks. Let's mention it in the changelog, though since we install master every day it doesn't make much difference.
|
Other than the question about announcing the numbering change, this LGTM. |
Adds the infrastructure (mock device and sources) to test ADQ digitizers and a basic set of tests for initialization and some auxiliary methods of the ADQ component.
I rather split this up, as the next methods will require significantly more complex tests.