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-30535: Update the docs, including API changes #58

Merged
merged 4 commits into from Sep 29, 2022
Merged

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Jul 12, 2022

No description provided.

@fred3m fred3m force-pushed the tickets/DM-30535 branch 3 times, most recently from a935587 to 48f3d0c Compare July 13, 2022 17:24
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 mix of suggestions and mostly minor fixes (typos, etc).

doc/lsst.meas.extensions.scarlet/models.rst Outdated Show resolved Hide resolved
doc/lsst.meas.extensions.scarlet/models.rst Outdated Show resolved Hide resolved
doc/lsst.meas.extensions.scarlet/models.rst Outdated Show resolved Hide resolved
doc/lsst.meas.extensions.scarlet/models.rst Show resolved Hide resolved
doc/lsst.meas.extensions.scarlet/models.rst Show resolved Hide resolved
@@ -442,7 +448,7 @@ def updateBlendRecords(blendData, catalog, modelPsf, observedPsf, redistributeIm
contains those columns.
"""
# We import here to avoid a circular dependency
Copy link

Choose a reason for hiding this comment

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

This seems like code smell...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an artifact of the current state of the code and is not worth fixing. Before the scarlet models were exported, the only place to make measurements on the deconvolved models was in the ScarletDeblendTask. But these are band-dependent measurements and the output from ScarletDeblendTask is now a single monochromatic catalog. DM-34558 will create a sub-task to make these (and potentially other) measurements, but until that is created there is really no appropriate place for these measurements to live.

python/lsst/meas/extensions/scarlet/io.py Show resolved Hide resolved
blend : `scarlet.lite.LiteBlend`
The full scarlet model for the blend.
"""
# Import here to prevent circular import
Copy link

Choose a reason for hiding this comment

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

As above, surely you can refactor these modules to avoid potential circular imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I agree and most of the refactoring in this ticket was to clean things like this up. However in this case I think that it would add an unnecessary module containing only a single function.

python/lsst/meas/extensions/scarlet/scarletDeblendTask.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/scarlet/scarletDeblendTask.py Outdated Show resolved Hide resolved
@fred3m fred3m force-pushed the tickets/DM-30535 branch 2 times, most recently from c3af6c7 to 9c30e37 Compare September 28, 2022 17:54
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 more minor suggestions for clarification.

While it is outside of the discussion in this tutorial, we have observed reddening residuals in edge-on galaxies and it is currently an active area of research to better model galactic (and intergalactic) dust using *scarlet*.

Because the *scarlet* model is multi-band, and the observations are not required (or recommended) to be matched to a common PSF, the model itself must exist in a space that can be convolved into the observed seeing in each band, which we will refer to as *model space*.
In order to be sufficiently well sampled we currently use a PSF for the model space that is a circular Gaussian with ``σ=0.8`` pixels, ensuring that our model image is well-sampled but smaller than the observed PSF in each observed band.
Copy link

Choose a reason for hiding this comment

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

Well, yes, but FFT convolution requires zero-padding the image, which I imagine can get quite memory intensive for a large blend without using sparse matrices. Direct convolution doesn't need padding or even a full-sized kernel image, amongst other things.

In that case setting :attr:`~lsst.meas.extensions.scarlet.ScarletDeblendConfig.setSpectra` to ``False`` is recommended.

.. note::
An interesting fact about optimization in scarlet is that the best initialization (in terms of 𝛘²) will not necessarily result in the fastest runtime or even the most optimal fit.
Copy link

Choose a reason for hiding this comment

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

Ok, then perhaps just mention that this is optimizer-dependent and not some quirk of other aspects of Scarlet.

.. image:: images/scarletModel3.png

While much improved, the models still aren't perfect.
Notice that the *scarlet* model for source 0 has stray flux to the upper left and lower right that (fortunately) doesn't matter in the flux re-apportioned model, however sources 1 and 2 have their flux slightly truncated due the the presence of their neighbor in both the *scarlet* models and flux re-apportioned models.
Copy link

Choose a reason for hiding this comment

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

Ok, then again I would note that this seems to be the optimizer's fault, rather than a problem with model selection or anything else the user could try to fix.

In this case the main assumption is that an astrophysical source can be modeled as a collection of components, where each component can be described by a single SED that is roughly constant but can vary in magnitude over its spatial extent (morphology).
This assumption can be justified physically by considering the structure of a galaxy.
In general the populations of stars in the bulge and disk are different, as well as perhaps additional populations of star forming regions, each with its own distinct SED.
However, the actual colors in each population are not that different, which is why parametric bulge-disk models (Sersic models, Spergel models, Gaussian mixture models, etc.) have been successfully used in deblending and photometric measurement.
Copy link

Choose a reason for hiding this comment

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

Ok, it sounded ambiguous so I suggest changing it to "the range of colors within each population is not that large", and potentially add something like "e.g. star-forming regions are blue, and bulges tend to be red".

@fred3m fred3m merged commit 7599f17 into main Sep 29, 2022
@fred3m fred3m deleted the tickets/DM-30535 branch September 29, 2022 19:16
@mwittgen mwittgen changed the title tickets/DM-30535: Update the docs, including API changes DM-30535: Update the docs, including API changes Oct 18, 2023
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