-
Notifications
You must be signed in to change notification settings - Fork 10
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
Rename DetectorArray
➡️ DetectorManager
plus Docstrings and Refactoring
#423
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
=======================================
Coverage 75.32% 75.32%
=======================================
Files 56 56
Lines 7810 7823 +13
=======================================
+ Hits 5883 5893 +10
- Misses 1927 1930 +3 ☔ View full report in Codecov by Sentry. |
DetectorArray
➡️ DetectorManager
plus Docstrings and Refactoring
f614413
to
8508722
Compare
Also in all corresponding file names. Also in variable names like detector_arrays -> detector_managers Rationale: I always found the existance of both DetectorList and DetectorArray quite confusing. The now-renamed class acts in a somewhat comparable way to the FOVManager or OpticsManager, thus I found it fitting to apply this naming convention also to this class. There are two occurences in IRDB package tests where this change will need to be incorporated to avoid breaking those tests. Also, some refactoring Make functions and attribute private that are not currently used outside the class anyway.
8508722
to
8c18271
Compare
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.
Good!
I'm wondering, should we do the initialization of ._detectors
earlier? Now they are only available after .readout()
is called.
Co-authored-by: Hugo Buddelmeijer <hugo@buddelmeijer.nl>
Hmm, perhaps not a bad idea. I'll include that if it doesn't break anything (which would surprise me but I've been surprised many times by this codebase). |
Had to catch the case where detector_list isn't passed, which happens in testing. Figured a warning wouldn't hurt, as this case shouldn't occur in regular operation.
Rename class
DetectorArray
➡️DetectorManager
and all corresponding file names.Also rename variable names like
detector_arrays
➡️detector_managers
and react to changes in test modules.Rationale
I always found the existence of both
DetectorList
andDetectorArray
quite confusing. The now-renamed class acts in a somewhat comparable way to theFOVManager
andOpticsManager
classes, thus I found it fitting to apply this naming convention also to this class.This was done after brief discussion with @astronomyk a few weeks ago, now rebased and finalized.
Consequences
There are two occurrences in IRDB package tests where this change will need to be incorporated to avoid breaking those tests. Other than that, the things changed here are rather internal, so unlikely to break any existing scripts.
Other Changes along the Way
Version Change?
This (and other things coming soon) might be considered worth of a minor version bump (aka v0.9.0).