Tickets/dm 2977 #10
Tickets/dm 2977 #10
Conversation
| { | ||
| initial.nComponents = 3; // use very rough model in initial fit | ||
| initial.optimizer.gradientThreshold = 1E-2; // with coarse convergence criteria | ||
| initial.optimizer.minTrustRadiusThreshold = 1E-2; | ||
| dev.profileName = "luv"; | ||
| exp.nComponents = 6; | ||
| exp.optimizer.maxOuterIterations = 250; | ||
| } |
There was a problem hiding this comment.
The changes in CModelControl appear to come from HSC-1264. This ticket also changed some default values in meas_extensions_multiShapelet, which has been removed from LSST, as part of commit aaf9922f2963a4a6750f60552742eaaa16839f72. Please verify that the changes to the default values in multiShapelet are appropriately set in the new LSST location, or are not needed in the LSST stack
There was a problem hiding this comment.
Good catch. I'd never actually synced up these parameters across the two codebases, since they're implemented differently. I'll add a commit to do that before merging.
|
Looking at the behavior of writeEllipses and readEllipse, in Model.cc the directionality of readParameters(nonlinearIter), and writeParameters(nonlenearIter) could be clearer. Are these functions using the supplied argument as an input to read/write data to the ellipse, or is the ellipse reading/writting data to the iterator? Consider a name change such that in the readEllipse function the writeParameters method of the ellipse would be replaced by something such as writeParametersToIterator or something equivalent. |
| @@ -442,6 +449,8 @@ struct CModelResult { | |||
| PTR(afw::detection::Footprint) initialFitRegion; ///< Pixels used in the initial fit. | |||
| PTR(afw::detection::Footprint) finalFitRegion; ///< Pixels used in the exp, dev, and linear fits. | |||
|
|
|||
| LocalUnitTransform fitSysToMeasSys; ///< Transforms to the coordinate system where parameters are defined | |||
There was a problem hiding this comment.
Does fitSysToMeasSys actually provide any functionality, i.e. does it actually 'fit' anything? It seems to be a class which only provides the data necessary for other functions to actually do the transforming. Consider renaming to avoid confusion, something such as SysToMeasSys(Info/Data/Mapping/etc)
There was a problem hiding this comment.
The confusion here is due to our naming conventions, which require lower-case letters for the start of variables: "FitSys" is the name of one of the coordinate systems, and "MeasSys" is the other, and this is a coordinate transformation object that goes between those two systems.
Cut/paste bug: time was being set to the number of iterations.
Time in sec was wrong by 10^18 due to incorrect scaling of nsec to sec.
These parameters, in combination with a change in meas_extensions_multiShapelet, result in a speed-up of about a factor of two, without compromising the photometric quality.
The previous (generally tighter) threshold was frequently misdiagnosing singular matrices with many solutions as problems with no solution. The threshold is now better tied to the size of the numbers being subtracted.
Sometimes we want the Hessian without the SR1 term. We'll just modify it in-place, because when we want to remove it, we'll typically have already done the optimization.
…e errors While the secant term is good overall, as it improves convergence, it can lead to incorrect Hessians, at least for the linear parameters, where we can easily compute the exact Hessian without it.
When an exception is thrown by in the final 2-component linear fit, we should ensure the failure flag is set even if its not a known failure mode.
Previous approach was simply incorrect, because it computed the uncertainties while holding the difference between components fixed, instead of their ratio (and those aren't the same, as I'd thought previously).
We use the Shape slot to feed our initial guess of the parameters, but we might want to try to fit even when the Shape slot measurement failed. In this case, we now use a multiple of the PSF moments.
When fitting objects that are pure noise (which now happens frequently when fitting in other than the detection band), fall back to the RMS to set the photometric scale for the problem when the flux is small.
Set shape flag when we fall back to PSF moments, (which is not a fatal error in this case), and never allocate it in forced photometry (since it's impossible for it to be set there).
Instead of bailing out early when we encounter a noncontiguous Footprint, we set flags, choose the largest (by area) contiguous region, and proceed.
This adds a new badCentroid flag and a number of new checks for bad inputs to CModel, mostly focusing on the problem of noncontiguous Footprints and centroids that aren't within their Footprints.
This accounts for the possibility that PSF moments as well as source moments can have invalid ellipses.
The new flag indicates failures in forced measurement that are simply due to the reference non-forced measurement failing.
We need to actually record the reference failure flag in the Result object before we can later use it to determine whether to set the badReference flag.
This was accidentally changed when merging review comments from the mainline LSST codebase back to the HSC fork in 7da4556.
Likelihood. Previously, we used a determinant-based average for the weights when not using per-pixel weights, which was never well-motivated, and got in the way of using Likelihood objects to computing more principled error estimates when fitting without using per-pixel weights. This changeset will temporarily break the calculation of CModel uncertainties (which apparently weren't well-tested enough to notice the change). That will be fixed in a future commit.
These options were never set to anything other than their defaults, and in fact doing so could cause serious problems in the algorithm's behavior.
This redefines the uncertainties by considering the fluxes as a continuous aperture measurements rather than least-squares fitting (this is more consistent with the fact that we ignore per-pixel variances). The previous calculation may have underestimated fluxes in the regime where photon noise from the source itself was important.
We attempt to extrapolate the flux to infinite, but we'd previously estimated the uncertainty using only the pixels we use in the fit. That can result in underestimated fluxes, so in this change we scale that uncertainty by the ratio of the extrapolated flux to the flux within the fit region only.
Previous code was not checking the case where inputs had NaNs or were entirely zero.
We now write diagnostic outputs in forced mode as well.
Also refactored to eliminate code duplication.
| if (fpSet.getFootprints()->size() > 1u) { | ||
| result.setFlag(CModelResult::INCOMPLETE_FIT_REGION, true); | ||
| for (std::size_t n = 0; n < fpSet.getFootprints()->size(); ++n) { | ||
| if ((*fpSet.getFootprints())[n]->contains(pixel)) { |
There was a problem hiding this comment.
I'm not sure about the type methods for FootprintSet, but if it is in some sort of vector format, would it be possible to iterate from beginning to end of it instead of looping over some index? I.E. I assume that you could start at the pointer position of *fpSet.getFootprints() and step forward, either by incrementing the pointer, or following a vector from start to end. This would save from calling getFootPrints() each iteration of the for loop.
There was a problem hiding this comment.
A good idea, but I'd rather not implement it yet, as doing it right will require use of auto and the HSC side doesn't yet build with C++11 features enabled. Until then (or until we've fully completed the HSC merge) I'd rather keep this code in sync with the HSC side.
We'll have to touch this code anyway sometime on DM-3559 (which will necessarily happen after the HSC merge is complete), so I'll plan to make the change there.
|
I don't know if it is a standard practice to write tests to verify error handling or not. In this code you have introduced a lot of logic on when and where to set various flags, and in what order. It would be nice if there was some test (unit or otherwise) to verify that all the flags get set appropriately under varying conditions, and that there is not weird collisions. This is probably not strictly necessary, and if you don't want to do that that is ok, but please verify the logic of setting flags and what order it may occur. I tried to do that as best as possible, but I do not have a full sense of the execution flow to this code. Alternatively if you agree that writing tests for this may be wise, but beyond the scope of this port, please create a ticket to address it in the future. It may be paranoid, but I fear getting into a situation where flags are not set correctly and bugs become difficult to trace, or alternatively something is not caught. |
TallJimbo
force-pushed
the
tickets/DM-2977
branch
from
b9ba606
to
bbe5b68
Compare
We've done a small amount of inspection to verify that these parameters (a second-order inner shapelet with a first-order outer shapelet) are no worse than two second-order shapelets. More work needs to be done to check those parameters further.
|
Renaming readParameters/writeParameters is DM-3664. Improving unit test coverage for flags is DM-3665. |
Creating pull request for leave review comments