DM-42157: Use new multiprofit config classes in pipelines #8
Conversation
The Jacobian is typically evaluated on all but 1-2 iterations.
pipelines/fit.yaml
Outdated
There was a problem hiding this comment.
There seems to be a lot of duplicate code here. Since this is in a pipeline yaml that is normal/fine, but I do worry that if the API changes in the future you'll be making a lot of redundant changes. I'm wondering if you could have a method to make the fit task that could reduce the boilerplate.
There was a problem hiding this comment.
I could maybe have convenience functions to generate some "standard" config instances - a point source is an obvious target - but most of the repetition is unavoidable.
The config_model lines can be moved from the python block to the yaml, I think. I don't know if it would really help readability given how nested they are.
Once this is moved to lsst, I'll solicit some more opinions on the pipelines. I don't want to commit to a particular simplification scheme right now.
| if sigma_subtract > 0: | ||
| sigma_subtract_sq = sigma_subtract * sigma_subtract | ||
| for param in self.psfmodel_data.parameters.values(): | ||
| if ( | ||
| isinstance(param, g2f.SigmaXParameterD) | ||
| or isinstance(param, g2f.SigmaYParameterD) | ||
| or isinstance(param, g2f.ReffXParameterD) | ||
| or isinstance(param, g2f.ReffYParameterD) | ||
| ): | ||
| param.value = math.sqrt(param.value**2 - sigma_subtract_sq) |
There was a problem hiding this comment.
You don't do anything sigma_subtract is negative. Is this normal/expected, or should something be added here to either throw an exception or update something?
There was a problem hiding this comment.
There's already a check on the field that it's finite and >0 (which I will change to be >=0, as it should have been to start with).
| x_max += 1 | ||
| y_max += 1 | ||
|
|
||
| if (x_min > 0) or (y_min > 0) or (x_max < img.shape[0]) or (y_max < img.shape[1]): |
There was a problem hiding this comment.
I'm not sure why you need this check. Shouldn't you always check that the source is inside the bbox and adjust it if not?
There was a problem hiding this comment.
x/y_min/max here are indices of columns with finite, unmasked data. It's checking if there are entire rows/columns at the edge of the bbox with bad data that can be cropped, since they'd have no impact on the fit anyway. This happens very rarely so I was considering dropping it entirely.
| # TODO: There ought to be a better way to not get the PSF centroids | ||
| # (those are part of model.data's fixed parameters) |
There was a problem hiding this comment.
Agreed. I wonder if adding a flag property to the parameter class would be sufficient.
There was a problem hiding this comment.
Hmm, what do you mean by that?
To be clear, in this section I'm trying to get fixed centroid parameters (for fixed centroid models) from sources but not the PSF. There is not any way to distinguish between the two, and adding one would be difficult and I think ultimately counterproductive (parameters shouldn't need to know which parametric object(s) they belong to). This is just slightly annoying because you can't get parameters from model.sources because it's a bare list of parametric objects, but I could just shuffle this off to a convenience function in multiprofit, or implement some kind of Sources/SourceList class in gauss2dfit.
There was a problem hiding this comment.
Honestly I'm not sure. I don't know if I meant to put that comment somewhere else or what, but it doesn't make sense to me either in this context.
| sigma_subtract_sq = sigma_subtract * sigma_subtract | ||
| for param in self.psfmodel_data.parameters.values(): | ||
| if ( | ||
| isinstance(param, g2f.SigmaXParameterD) |
There was a problem hiding this comment.
Quick comment that isinstance takes mutliple classes so you can do this in one call.
isinstance(param, g2f.SigmaXParameterD | g2f.SigmaYParameterD | g2f.ReffXParameterD)for example (not listing all 4 in my comment).
There was a problem hiding this comment.
I almost can't believe I didn't know that... thanks.
taranu
force-pushed
the
tickets/DM-42157
branch
from
c5fa8b6
to
a93d281
Compare
There are numerous changes and additions to the pipelines, including better support for fixed-centroid models, and bug fixes for coordinate systems and x/y to ra/dec conversions.