tickets/DM-24862 #24
tickets/DM-24862 #24
Conversation
The API changed to require all `Frame` and `Observation` classes to contain a `channels` parameter, and the morphology and default models are no longer in the full model frame but instead in smaller boxes. So this commit fixed the scarlet tests.
|
|
||
| __all__ = ["deblend", "ScarletDeblendConfig", "ScarletDeblendTask"] | ||
|
|
||
| logger = lsst.log.Log.getLogger("meas.deblender.deblend") | ||
|
|
||
|
|
||
| def _checkBlendConvergence(blend, f_rel): | ||
| """Check whether or not a blend has converged |
There was a problem hiding this comment.
Needs numpy docs for args and returns (even if they're fairly straightforward)
There was a problem hiding this comment.
I don't think this is true for private methods. It's certainly common practice to not include arguments and parameters for private methods and the only place this is mentioned in the dev guid is
def _private(self):
"""Do something internally.
By default private members are not included in the HTML docs either.
Notes
-----
Private members are any methods or attributes that start with an
underscore and are *not* special. By default they are not included in
the output.
However, you should still provide docstrings for private members to
document code for internal developers.
"""
pass
Looking at the other method in that section (https://developer.lsst.io/python/numpydoc.html#complete-example-module) I feel like this implies that docstrings are needed but in a case where the parameters are obvious we don't need to be so verbose. But if you can find somewhere that says otherwise...
| The images, psfs, etc, of the observed data. | ||
| bbox : `lsst.geom.Box2I` | ||
| The bounding box of the parent footprint. | ||
| center : `tuple` |
There was a problem hiding this comment.
You could put tuple [float]. I'm not sure if numpydoc allows specifying the length exactly though.
| If a source fails to initialize with `maxComponents` then it | ||
| will continue to subtract one from the number of components | ||
| until it reaches zero (which fits a point source). | ||
| If a point source cannot be fit then the source is skipped. |
There was a problem hiding this comment.
The docstring for fallback implies that this will only happen if fallback = True, so you should probably mention that here.
| Whether or not to decrease the number of components for sources | ||
| with small bounding boxes. For example, a source with no flux | ||
| outside of its 16x16 box is unlikely to be resolved enough | ||
| for multiple components, so a single source can be used. |
There was a problem hiding this comment.
Maybe just explicitly state how downgrade lowers maxcomponents? You can explain the logic in a Notes section.
There was a problem hiding this comment.
I don't want to be too explicit because the exact implementation might change over time and I don't want the docs and the code to get out of sync.
| logger.warning("Could not initialize") | ||
| raise ValueError("Could not initialize source") | ||
| if hasEdgeFlux(source, edgeDistance): | ||
| msg = "Could not initlialize source at {} with {} components".format(center, maxComponents) |
| # FYI: The `scarlet.Box` class implements the `&` operator | ||
| # to take the intersection of two boxes. | ||
|
|
||
| # Get the PSF size and radii to grow the |
| # Always use a real space convolution to limit artifacts | ||
| model = observation.convolve(model, convolution_type="real").astype(dtype) | ||
| # Update xy0 with the origin of the sources box | ||
| xy0 = Point2I(overlap.origin[-1]+xy0.x, overlap.origin[-2]+xy0.y) |
There was a problem hiding this comment.
The style guide says + and - should have spaces, so I'd suggest staying consistent with that (there are spaces just above).
Update to latest proxmin and scarlet.