meas_base review for DM-1161 #13
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some reminders to myself as I continue to work through the review comments:
|
| import unittest | ||
| import lsst.utils.tests | ||
| import numpy |
There was a problem hiding this comment.
This numpy import isn't used.
TallJimbo
force-pushed
the
tickets/DM-1161
branch
2 times, most recently
from
aecccee
to
82a221e
Compare
Underscores should be used as a namespace separator, with CamelCase used to separate words.
This was the cause of mysteriously large differences between the so-called truth values and our measurements - we weren't actually drawing the Gaussians we claimed we were drawing when making the test data.
Classification should still run when the error fields it might have used aren't present but it isn't configured to use them anyway.
This is a major refactoring of the test data generation code in the tests.py module, which in turn required major changes to most of the algorithm tests. The good news is that these are now more concise, more complete, and in at least some cases, more strict. In the process, testSingleFrameMeasurement.py was replaced with testNoiseReplacer.py (as noise replacement was the only thing begin tested there that wasn't already being tested elsewhere). In addition, testForced.py was replaced with new tests for forced measurement in the test files for the flux algorithms.
While the rest of getFixedMomentsFlux() was updated at some point to work in parent coordinates, the call to set_amom_bbox (which still uses LOCAL) was not.
We now correctly fill the flux and uncertainty fields, and no longer create fields to save uncertainty on xy4 or the centroid.
We can reduce some boilerplate by templating on the image type rather than the pixel type with explicit overloads for Image and MaskedImage.
This gets SdssShapeImpl out of the interface that GaussianFlux depends on, bringing us one step closer to getting rid of it. This also addresses a problem with GaussianFlux overestimating its errors; instead of relying on the calc_fisher routine in SdssShape for that, we now estimate it directly from propagation of errors for flux only. This is a difference in philosophy as well as implementation, as calc_fisher attempted to propagate uncertainty in the moments into the uncertainty in the flux, but from GaussianFlux's perspective the moments are held fixed.
This change weaks several test thresholds to pass with typical (rather than "lucky") random number generator seeds, while tightening others as much as possible to meet the same criteria.
This was a holdover from SDSS, where this 4th moment could be used as an input to a higher-level algorithm to estimate PSF-corrected ellipticities from the second moments. It has now been completely superseded by the HSM shape package.
Prior to this change the "careful" parameter had no effect, which appeared to be a bug. After fixing this, the results - as tested on the stack demo package - were clearly worse, in the sense that objects that previously had reasonable moments had moments that were smaller than the PSF. So, instead, I'm just removing that option entirely, which preserves the old behavior while removing the faulty logic.
I haven't cleaned this up as much as I could have (by e.g. moving the code getAdaptiveMoments directly into SdssShape::computeAdaptiveMoments), because I felt preserving the connection to past versions of the code was more important. Ultimately, we'll be replacing SdssShape with a new-from-scratch adaptive moments algorithm, so I'm not concerned about the long-term implications of the messy code here.
This changes results slightly in lsst_dm_stack_demo (as the the region of pixels used in the fit can differ by 1 pixel due to different rounding).
TallJimbo
force-pushed
the
tickets/DM-1161
branch
from
82a221e
to
b1f4754
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.