Add DetectorData API

[philsmt]

As the CAL team has been refactoring the injection code away from the legacy calibrationDBRemote API, we noticed that handling PDU details is somewhat cumbersome and requires replicating non-obvious RESTful requests in many places. As we do not want to add further auxiliary functions to CalCatAPIClient, this PR adds an API accompanying CalibrationData by DetectorData.

[codecov]

Codecov Report

❌ Patch coverage is 91.91176% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.14%. Comparing base (1dbf021) to head (8767ba3).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/extra/calibration.py	91.91%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
+ Coverage   72.76%   73.14%   +0.37%     
==========================================
  Files          35       35              
  Lines        6136     6248     +112     
==========================================
+ Hits         4465     4570     +105     
- Misses       1671     1678       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[philsmt]

Pointing at caldata-from-corrections until it is merged to fix the CI.

Rebased on top of master given that PR is merged.

[takluyver]

Is the goal to replace CalibrationData.module_details and .detector_name with a DetectorData object? If so, we might need to do something about the .from_correction(..., use_calcat=False) case, where we don't have most of the info this assumes, just the bare minimum of detector name, aggregator names and PDU names.

[philsmt]

Is the goal to replace CalibrationData.module_details and .detector_name with a DetectorData object?

I suppose that would be neat, but quite the refactor around CalibrationData. We could also emulate module_details via the DetectorData for compatibility. I might want to start out with a detector property on CalibrationData constructing the object at the (potential) cost of re-querying, or try to construct it with the information present.

[takluyver]

In the context of using DetectorData in CalibrationData, we discussed whether it should be possible to select down modules within DetectorData. I think I'm OK with saying that DetectorData may represent part of a detector. Possibly it could have a detdata.base object pointing to the original object with all modules, like numpy array views. But I'm not sure when we'd use that.

Another option would be to keep DetectorData whole, with all the modules we originally found, and have caldata.selected_modules indicate which modules are in use. 🤷

[philsmt]

Apart from addressing the comments, I tried to base the CalibrationData on top of DetectorData.

I tried to do this in a minimalistic approach by preserving all business logic and only emulating detector_name and module_details underneath. It passes the test suite just fine, what are your thoughts?

Technically there are some differences in obscure fields like detector_type_id and such, but I don't expect this to matter in practice.

[philsmt]

I cleaned up the history now, could you please have a look at the remaining discussions on whether they're actionable?

[takluyver]

Let's not guess here. Either we preserve two separate values from what CalCat returns so we can return them separately, or else just return a single module_number field. It just creates confusion otherwise.

[philsmt]

Oops, this was probably a preliminary measure when trying out the compatibility layer.

Does it ever make sense to look at module numbers different from when the CCV was taken?

[philsmt]

As discussed, I removed module_number_at_ccv_begin_at for now.

[takluyver]

The cassette files for the tests seem to have ~doubled in size (or more):

I think --record-mode=all maybe adds the new requests to the existing ones? Though I'm a bit confused, because some of the old requests seem to change as well. I think the rewrite record mode will completely replace the previous responses, so the files don't grow. This is what the comment in test_calibration suggests:

EXtra/tests/test_calibration.py

Lines 22 to 25 in c4e8708

	# Most of these tests use saved HTTP responses by default (with pytest-recording).
	# To ignore these & use exflcalproxy, run pytest with the --disable-recording flag.
	# To record responses for a new test making HTTP requests, pass --record-mode=once.
	# To update the saved cassettes from exflcalproxy, pass --record-mode=rewrite.

[philsmt]

Hmm I see, I tried to clean up by deleting and generating them again from scratch. Does this look better?

[takluyver]

Thanks, indeed it now looks like the cassette files stay roughly the same size.

LGTM

[takluyver]

I've rebased onto master to resolve some conflicts. I think this is ready to be merged.

[takluyver]

Nitpicking, but having both from_id and from_identifier feels like setting up ongoing low-level confusion, and the one people probably want more often is the longer one.

How about we call this one from_calcat_id()?

[philsmt]

Yeah, I can see that confusion.

How about from_numerical_id() as already written in the docstring?

[takluyver]

That's OK by me. I think 'numeric' also works if you want to shorten it slightly.

[philsmt]

Good idea. I'll take your earlier LGTM then once the pipeline finishes.