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

DM-38432: Implement scarlet_lite stand alone package #70

Merged
merged 1 commit into from Sep 30, 2023

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Apr 17, 2023

No description provided.

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.

Broadly fine, but I have some more clarification requests and small fixes.

python/lsst/meas/extensions/scarlet/io.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/scarlet/io.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/scarlet/io.py Outdated Show resolved Hide resolved
totalBands = len(spectrum.x)
morph = FixedParameter(factorizdedData.morph)
factorized = FactorizedComponent(
bands=("dummy", ) * totalBands,
Copy link

Choose a reason for hiding this comment

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

Is it surprising that the band name not matching "band" above didn't cause any problems, or are band names just labels that get ignored assuming that users keep bands in a consistent order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The labels are meaningless when the model is only used in a single band. We only care about the band names if we are looking at the full multiband models.

modelData: scl.io.ScarletModelData,
catalog: SourceCatalog,
band: str,
redistributeImage: afwImage | None = None,
Copy link

Choose a reason for hiding this comment

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

This kind of sounds more like a boolean arg. Maybe redistributingImage instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to imageForRedistribution.

Returns
-------
heavy:
The footprint (possibly multiband) containing the model for the source.
Copy link

Choose a reason for hiding this comment

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

Is MultibandFootprint actually a HeavyFootprintF? If not, the return value type hint should be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be either one, thanks for pointing this out. I updated the docstring and type hint.

tests/test_deblend.py Outdated Show resolved Hide resolved
tests/test_deblend.py Outdated Show resolved Hide resolved
self.assertEqual(config.version, "lite")

# Check that the PSF and other properties are the same
assert_almost_equal(oldModelData.psf, modelData.psf)
Copy link

Choose a reason for hiding this comment

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

Are you expecting machine epsilon level differences or larger ones (in which case this might be too sensitive of a check)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should be identical, but I made them machine level just to be safe.

self.assertTupleEqual(oldBlendData.shape, blendData.shape)
self.assertTupleEqual(oldBlendData.psf_center, blendData.psf_center)
self.assertTupleEqual(tuple(oldBlendData.sources.keys()), tuple(blendData.sources.keys()))
assert_almost_equal(oldBlendData.psf, blendData.psf)
Copy link

Choose a reason for hiding this comment

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

This is the PSF per blend, which is different from the oldModelData.psf above, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. oldModelData.psf and modelData.psf is the model PSF that all of the blends are generated in.

@fred3m fred3m force-pushed the tickets/DM-38432 branch 2 times, most recently from ca0b4c6 to a5fdf76 Compare September 28, 2023 14:34
@fred3m fred3m merged commit e5298a2 into main Sep 30, 2023
2 checks passed
@fred3m fred3m deleted the tickets/DM-38432 branch September 30, 2023 15:01
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