tickets/DM-23364 #19
tickets/DM-23364 #19
Conversation
fred3m
force-pushed
the
tickets/DM-23364
branch
from
5b6e4ba
to
eb4e141
Compare
| doc=("Calculate exact Lipschitz constant in every step" | ||
| "(True) or only calculate the approximate" | ||
| "Lipschitz constant with significant changes in A,S" | ||
| "(False)")) |
There was a problem hiding this comment.
These seem like they should be on their own commit, I dont see how it relates to the edge work
There was a problem hiding this comment.
Agreed. I came across obsolete parameters when I was adding the edge ones and rather than forget to remove them I deleted them right away. But it seems like a small enough change to not warrant the time that it would take to pick lines for a separate commit.
| @@ -264,8 +250,6 @@ class ScarletDeblendConfig(pexConfig.Config): | |||
| processSingles = pexConfig.Field( | |||
| dtype=bool, default=False, | |||
| doc="Whether or not to process isolated sources in the deblender") | |||
| storeHistory = pexConfig.Field(dtype=bool, default=False, | |||
| doc="Whether or not to store the history for each source") | |||
There was a problem hiding this comment.
same with all these changes
| symmetric=False, monotonic=True, | ||
| thresh=5, components=1): | ||
| symmetric=False, monotonic=True, | ||
| thresh=5, components=1, edgeDistance=1, shifting=False): |
There was a problem hiding this comment.
This is not wrong per-say, but keep in mind that as the number of function arguments grows, it becomes hard to keep track of and or call. At some length it is worth considering a dedicated class to set and pass.
There was a problem hiding this comment.
Yeah, I think this is just a stylistic question. I only prefer using a class if the majority of the components are going to be used in multiple function calls. In this case, while there are a lot of arguments I'm not a big fan of using a class that is only called once in a codebase.
| # (roughly 5 pixels from the edge). This results in poor | ||
| # deblending of the edge source, which for bright sources | ||
| # may ruin an entire blend. So we reinitialize edge sources | ||
| # to allow for shifting and return the result. |
There was a problem hiding this comment.
Note that while shifting may be defined w/n scarlet, it does not seem to be defined in this package, which makes this comment less useful. Consider either a brief description here (and or somewhere else in this package) or a link to scarlet docs where shifting is talked about.
| model = source.get_model()[band] | ||
| for edge in range(edgeDistance): | ||
| if ( | ||
| np.any(model[edge-1] > 0) or |
There was a problem hiding this comment.
are scarlet models somehow 1 based indexing? if edge in the loop is zero, 0-1 is -1 and so this will test the last pixel, then the 0th etc. Is this what you were intending?
There was a problem hiding this comment.
Note the assert. So the minimum value of edgeDistance is 1, which describes the edge.
I updated the docstring to make this clear.
| """hasEdgeFlux | ||
|
|
||
| Determine whether or not a source has flux within `edgeDistance` | ||
| of the edge. |
There was a problem hiding this comment.
Are these the edges of a box containing the entire footprint to deblend, or a box only containing an individual source, i.e. a smaller view?
There was a problem hiding this comment.
Good question. source.get_model returns an array that is the size of the blend, with the source injected into it. So this only tests against a true image edge.
|
|
||
| # Use the first band that has a non-zero SED | ||
| band = np.min(np.where(source.sed > 0)[0]) | ||
| model = source.get_model()[band] |
There was a problem hiding this comment.
if you did something like:
edges = np.ones(model.shape)
edges[edgeDistance+1:-edgeDistance-1, edgeDistance+1:-edgeDistance-1] = 0
return np.sum(model*edges)>0
you could avoid all the ifs, and reduce the number of implicit loops
There was a problem hiding this comment.
True, but for N = length of a side, that code scales as N^2 whereas the current algorithm scales with N. I verified this in a jupyter notebook.
The code now checks for sources initialized with NaN values in their morphology, which causes scarlet to fail.
Before this fix all MultiComponentSource objects threw an error because MultiComponentSource does not have an `sed` attribute. The code was modified to check for the optimal band to use for multicomponent sources.
Allow sources on the edge with poorly constrained positions to shift. This ticket also marks sources on the edge to notify users downstream.