Skip to content
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-32436: implement scarlet lite #47

Merged
merged 3 commits into from Feb 16, 2022
Merged

tickets/DM-32436: implement scarlet lite #47

merged 3 commits into from Feb 16, 2022

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Jan 26, 2022

No description provided.

fred3m added 2 commits January 26, 2022 11:37
`modelToHeavy` was modified to remove an incorrect thresholding
algorithm used to truncate convolved scarlet models and replace
it with an algorithm that grows the deconvolved footprint with
the rectangular PSF.
Deblended children near the edge of an image can have their
footprints dilated outside of the exposure by the PSF width.
This commit ensures that the dilated model is always contained
fully in the exposure. This is valid, as later in pipe_tasks
the science pipelines flag edge sources so it isn't expected
that all of their flux is contained in the image.
Copy link

@taranu taranu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few issues and some requests for clarification.

@@ -771,17 +964,15 @@ def deblend(self, mExposure, catalog):
try:
t0 = time.time()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internet suggests time.monotonic or time.perf_counter, in case of system time changes (doubt that comes up much, but anyway).

@@ -807,7 +998,10 @@ def deblend(self, mExposure, catalog):
continue

# Update the parent record with the deblending results
logL = blend.loss[-1]-blend.observations[0].log_norm
if self.config.version == "scarlet":
logL = -blend.loss[-1]+blend.observations[0].log_norm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be spaces around the plus: https://developer.lsst.io/python/style.html#binary-operators-should-be-surrounded-by-a-single-space-except-for
(that goes for other lines in this file too)

"""Convert a scarlet model to a `MultibandFootprint`.
Parameters
----------
source : `scarlet.Component`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a scarlet.Source?

@@ -146,3 +146,78 @@ def modelToHeavy(source, mExposure, blend, xy0=Point2I(), dtype=np.float32):
model = MultibandImage(mExposure.filters, model, valid.getBBox())
mHeavy = MultibandFootprint.fromImages(mExposure.filters, model, footprint=foot)
return mHeavy


def liteModelToHeavy(source, mExposure, blend, xy0=Point2I(), dtype=np.float32, useFlux=False):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if you don't mutate xy0 currently, this should still be xy0=None and conditionally initialized to Point2I() below (https://developer.lsst.io/python/style.html#a-mutable-object-must-not-be-used-as-a-keyword-argument-default). Also, if you don't actually need a Point2I, you could just pass x0 and y0 floats.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't Point2I immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, apparently not. Yes, I'll fix this.

origin = source_box.origin
else:
source_box = source.bbox
shape = (source_box.shape[0], source_box.shape[1] + py, source_box.shape[2] + px)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you only need to grow the box by 2*dh, 2*dw instead of py, px (if the psf shape is odd)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I think that you're right.

@fred3m fred3m force-pushed the tickets/DM-32436 branch 2 times, most recently from 4ef56e3 to eeb47e8 Compare February 9, 2022 18:40
@fred3m
Copy link
Contributor Author

fred3m commented Feb 9, 2022

Ok, I think that I addressed everything now. Once approved I'll run Jenkins before merging.

@fred3m fred3m merged commit 2199095 into main Feb 16, 2022
@fred3m fred3m deleted the tickets/DM-32436 branch February 16, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants