DM-24738: Add background offset tracking to fgcmcal #77
Conversation
| # Ignore warnings, we will filter infinites and nans below | ||
| np.warnings.simplefilter("ignore") | ||
|
|
||
| instMagIn = -2.5*np.log10(df[self.config.apertureInnerInstFluxField].values[use]) |
There was a problem hiding this comment.
Please call it instMagInner and instMagOuter to not confuse the variables with input and output magnitudes
| deltaAperOuterRadiusArcsec = pexConfig.Field( | ||
| doc="Outer radius used to compute deltaMagAper (arcseconds).", | ||
| dtype=float, | ||
| default=0.0, |
There was a problem hiding this comment.
Is this a sensible default? I'd have hoped that both deltaAperInnerRadiusArcsec and deltaAperOuterRadiusArcsec be positive and not equal.
There was a problem hiding this comment.
fgcmcal itself doesn't know the radii so I don't want to set to an arbitrary number. (Though this is actually gettable through a complicated metadata transaction that I did not want to add, though it could be.). The fgcm code will take 0s to mean that the output should be "unnormalized" units and I added this to the docstring. I think that the key is that these should be set in the various pipeline configs for a given dataset.
There was a problem hiding this comment.
In that case, the optional keyword argument should be set to False to indicate that a user must ensure that this is provided (I don't know what default would do for a non-optional config field though)
There was a problem hiding this comment.
I'm not sure about that. You don't need to set this if you aren't doing any of the delta-aper computations. And the code will run. But maybe what I need to do is put some code in the validation field that ensures that these are set to positive numbers if any of the do options are set. So I'll do that.
There was a problem hiding this comment.
Ok, that is tricky then. Checking for positivity within a validate method sounds good.
python/lsst/fgcmcal/fgcmFitCycle.py
Outdated
| dtype=bool, | ||
| default=False, | ||
| ) | ||
| doComputeDeltaAperStars = pexConfig.Field( |
There was a problem hiding this comment.
The stars in the name can be misleading, making one expect there is an equivalent config for galaxies perhaps? May be this should be doComputeDeltaAperMean as the doc says?
There was a problem hiding this comment.
Well, there are no galaxies in fgcmcal, so and "stars" is used all over the place. I think that maybe the confusion is the same as below ... this should be doComputeDeltaAperPerStar I think, to go with doComputeDeltaAperPerVisit, doComputeDeltaAperPerCcd.
| dtype=bool, | ||
| default=False, | ||
| ) | ||
| doComputeDeltaAperPerCcd = pexConfig.Field( |
There was a problem hiding this comment.
At the visit level, there was no "per" in config name. I don't have a strong inclination either way, but either both or neither should have "per".
There was a problem hiding this comment.
I'm adding per across the board.
python/lsst/fgcmcal/fgcmFitCycle.py
Outdated
| default=True, | ||
| ) | ||
| doComputeDeltaAperMap = pexConfig.Field( | ||
| doc="Do the computation of delta-aper maps?", |
There was a problem hiding this comment.
The doc should says that this config matters only when doComputeDeltaAperStars is True. Similarly for doComputeDeltaAperPerCcd.
There was a problem hiding this comment.
Very good point, I'll add that. And on further review I realized that it can compute the per-ccd values even if the per-star values weren't computed. So I've updated the fgcm code, and I'm going to make sure the docstrings are clear here.
| dtype=int, | ||
| default=2, | ||
| ) | ||
| deltaAperFitPerCcdNx = pexConfig.Field( |
There was a problem hiding this comment.
Do these kick in only when doComputeDeltaAperPerCcd is True?
erykoff
force-pushed
the
tickets/DM-24738
branch
from
bcf75a4
to
6a681c2
Compare
By default, only the per-star and global offsets are computed because of the extra computation time. These values can also be easily computed in a post-processing step.