meas_base review for tickets/DM-3182 #21
Conversation
TallJimbo
changed the title
|
I'm not convinced moving BaseMeasurementTask into its own file is an improvement, at least not on its own - before, we had a nice symmetry between base.py, forcedMeasurement.py, and sfm.py, where each held both the plugin base class and the task driver (and their configs), and that's now been lost. Admittedly, base.py also had a bunch of other helper code, but if you're going to move BaseMeasurementTask into its own file, I think you should do the same with the other two files, and rename what's left over to e.g. basePlugin.py. I'm ambivalent as to whether you do that or just leave the organization as it was, but I'd rather not land halfway in between. |
|
|
||
| self.schema must be available when this is called | ||
| """ | ||
| self.makeSubtask("applyApCorr", schema=self.schema) |
There was a problem hiding this comment.
I don't think this provides enough convenience/code-sharing to be worthwhile, but I don't care deeply enough about it to fight it if you feel strongly about it.
There was a problem hiding this comment.
It is safety I'm worried about. Without it I see no reason that people who author new subclasses will understand the need to instantiate the aperture correction subtask. However, I'll remove it for now, since it does add clutter.
|
On the question of whether to just extract the |
|
Regarding adding baseMeasurment: I agree about the lost symmetry. I will move the two base plugin classes into baseMeasurement.py, which restores symmetry and leaves base.py for really basic stuff and avoids a circular import, which was the primary motivation for the reorganization. I think renaming sfm.py to singleFrameMeasurement.py would also be an improvement for readability. |
|
I am very reluctant to add apCorrMap as an argument to the measure methods until we demonstrate a need for it. I had started out using it but I feel it results in clutter and needless complexity. It will be very easy to add later if it proves useful. |
|
Please go ahead and rename sfm.py if you're up to it. It sounds like the right place to do it. Maybe we should reconsider renaming base.py, too, if it's just the utility code now? Leaving out the apCorrMap argument is fine with me. |
Move BasePlugin, BasePluginConfig, BaseMeasurementTask and BaseMeasurementConfig to a new module baseMeasurement.py in order to allow BaseMeasurementTask access to ApplyApCorrTask. Without this refactoring it would be very difficult to avoid a circular import. This change is arguably an improvement because it moves higher-level code out of base.py and moves BaseMeasurementTask into a more obvious module (one that analogous to sfm.py and forcedMeasurement.py)
r-owen
force-pushed
the
tickets/DM-3182
branch
2 times, most recently
from
7981a53
to
4d91e80
Compare
BaseMeasurementTask and its subtasks now apply aperture correction as part of the run method. Most of the work is done in BaseMeaurementTask._applyApCorrIfWanted, a simple private wrapper around self.apCorrMap.run that only applies aperture corrections if wanted.
- Eliminate use of "from X import *" - Remove unused imports - Add a missing math import in one test - Add __all__ to some modules to avoid importing too many symbols - Import a few more files in __init__ (whose symbols were being pulled in due to lack of "__all__" in other modules). - Reorder a few imports to meet LSST standards - Stop assigning to variables that are not used; this showed up in a few unit tests and I left the computation in place, since performing the computation looked like it would probably be a useful test.
No description provided.