DM-23030: Modify Photometry SDM Functor to use stored calibration value. #355
Conversation
morriscb
force-pushed
the
tickets/DM-23030
branch
from
5d379c6
to
1841575
Compare
There was a problem hiding this comment.
Mostly documentation fixes, otherwise looks good. Please merge the second commit into the first, as it's clear that the first commit is not self-contained.
python/lsst/pipe/tasks/functors.py
Outdated
|
|
||
| @property | ||
| def name(self): | ||
| return 'mag_{0}'.format(self.instFluxCol) |
There was a problem hiding this comment.
I'm not sure we use format much in the Stack, consider using f-strings for consistency:
| return 'mag_{0}'.format(self.instFluxCol) | |
| return f'mag_{self.instFluxCol}' |
python/lsst/pipe/tasks/functors.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| instFlux : `numpy.ndarray` or `pd.Series` |
There was a problem hiding this comment.
I don't think Sphinx can parse namespace aliases. Does this work when you build the documentation and look at it in a browser? If not, use pandas instead of pd.
python/lsst/pipe/tasks/functors.py
Outdated
| calibFlux : `numpy.ndarray` or `pd.Series` | ||
| Array of calibrated flux measurements. |
There was a problem hiding this comment.
Can you say what happens if one input is an ndarray and the other is a Series? What do you get back?
There was a problem hiding this comment.
Comes out as Series.
python/lsst/pipe/tasks/functors.py
Outdated
| return -2.5 * np.log10(self.instFluxToNanojansky(instFlux, localCalib)) + self.logNJanskyToAB | ||
|
|
||
| def instFluxErrToMagnitudeErr(self, instFlux, instFluxErr, localCalib, localCalibErr): | ||
| """Convert instrument flux to nanojanskys. |
There was a problem hiding this comment.
Incorrect description; you're converting the errors.
|
|
||
|
|
||
| class LocalNanojansky(LocalPhotometry): | ||
| """Compute calibrated fluxes using the local calibration value. |
There was a problem hiding this comment.
It took me a while to figure out what's going on here. I suggest:
- Clarifying in the
LocalPhotometrydocumentation that it's an abstract base class with general calibration code, and not itself a completeFunctor. - Removing from
LocalPhotometrythenameandcolumnsproperties, which are always overridden in the functors anyway. - Clarifying in
LocalNanojansky,LocalNanojanskyErr,LocalMagnitude, andLocalMagnitudeErrthat these are pieces of a bigger system (e.g., by having a See also section that links between the flux and error versions).
tests/test_functors.py
Outdated
| rtol=0)) | ||
|
|
||
| # Test functors | ||
| func = LocalNanojansky("base_PsfFlux_instFlux", |
There was a problem hiding this comment.
Why not have one test case for each class instead of putting everything into a very long method? You'll get more information when you run the tests, it'll be easier to make changes to the test code, and you won't have to worry about weird side effects from shared variables like func and val.
There was a problem hiding this comment.
Moved the tests into their own function so the duplicated code is reduced. Considering how closely related the functors are and how they dependent they are the previously tested methods, I'm fine with leaving them here for now.
morriscb
changed the title
|
Hey @kfindeisen, think I've gotten through all of your comments. Take a look and let me know if I missed anything. I'll squash the commits once you have a look. |
python/lsst/pipe/tasks/functors.py
Outdated
| See also | ||
| -------- | ||
| LocalPhotometry | ||
| LocalNanojansky | ||
| LocalNanojanskyErr | ||
| LocalMagnitude | ||
| LocalMagnitudeErr |
There was a problem hiding this comment.
I think this might be a bit overkill. You can at least remove LocalPhotometry, because the docs always have an automatic link to the base class.
Add columns definition returns. Add docstrings. Add inifial localPhotometry unittest. Debug localPhotometry functor. Switch to astropy. Create unittest for LocalPhotometry. Debug unittest Add functor unittests. Add functor value testing. Debug unittest Fixup review requests. Change to f strings. Remove extra see also.
morriscb
force-pushed
the
tickets/DM-23030
branch
from
afd9c5c
to
1ded102
Compare
No description provided.