New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tickets/DM-8694: Refactor deblender internal API #15
Conversation
DM-8677 notes that the assignStrayFlux is not used, however findStray flux has the same functionality. So it was decided to remove findStaryFlux and use assignStray flux in its place. At the recommendation of @jbosch findStaryFlux was removed, as opposed to deprecated as the ticket sugggested.
(Commenting to be nosy.) Are there associated docs changes? Looking forward to trying the new API. |
@josePhoenix Yes, see the docstrings in the modified baseline.py, and plugins.py. If you think anything is not clear, this is a good time to bring them up while this ticket is in review. |
You can also look at |
Masked image containing the ``footprint`` in each band. | ||
psf: list of `afw.detection.Psf`s | ||
Psf of the ``maskedImage`` for each band. | ||
psffwhm: list pf `float`s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"pf" - > "of"
maxNumberOfPeaks: `int`, optional | ||
If positive, the maximum number of peaks to deblend. | ||
If the total number of peaks is greater than ``maxNumberOfPeaks``, | ||
then only the brightest ``maxNumberOfPeaks`` sources are deblended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes the peaks are sorted by flux, which we usually do, but may not always be the case. Also applicable to documentation in newDeblend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks for catching this.
for f, templateSum in templateSums.items(): | ||
self.deblendedParents[f].templateSum = templateSum | ||
|
||
class DeblendedParent(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this class need a docstring?
deblendedPeak = DeblendedPeak(peaks[idx], idx, self) | ||
self.peaks.append(deblendedPeak) | ||
|
||
def updateFootprintBbox(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "updateFootprintBBox" to match convention?
"""Update the bounding box of the parent footprint | ||
|
||
If the parent Footprint is resized, for example when `meas.deblender.baseline.deblend` | ||
has `patchEdges=True`, it will be necessary to update the bounding box of the footprint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this comment as I don't see a call to this function when patchEdges=True
. Also, do you actually want to update these values when patchEdges=True
, because only the template will change size and not the image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I moved self.imbb = self.img.getBBox()
to the init function and left only the lines that store the new footprint bounding box.
------- | ||
modified: `bool` | ||
Since all of the templates are modified by ``medianSmoothTemplates``, | ||
``modified`` is always ``True``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all your objects are psfs or have been set to be skipped, shouldn't this return false? This question also applies to a number of other plugins below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
_weightTemplates(debResult.deblendedParents[fidx]) | ||
return False | ||
|
||
def _weightTemplates(dp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to have this defined in a separate function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There may be a case where one doesn't run reconstructTemplates
, however if reconstructTemplates
is executed then weightTemplates
must be called each time a degenerate template is removed.
def _weightTemplates(dp): | ||
"""Weight the templates to best match the parent Footprint in a single filter | ||
|
||
This includes weighting both regular footprints and point source templates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"footprint" -> "template"
"""Apportion flux to all of the peak templates in each filter | ||
|
||
Divide the ``maskedImage`` flux amongst all of the templates based on the fraction of | ||
flux assigned to each ``templateFootprint``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be just "template", not "templateFootprint", or you could use "templateImage"
Any stray-flux portion less than ``clipStrayFluxFraction`` is clipped to zero. | ||
getTemplateSum: `bool`, optional | ||
As part of the flux calculation, the sum of the templates is calculated. | ||
If ``getTemplateSum==True`` then the sum of the templates is stored in the result (a `PerFootprint`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PerFootprint -> "DeblenderResult"
No description provided.