pmelchior / scarlet

hyperspectral galaxy modeling and deblending
MIT License
50 stars 22 forks source link

Init multi #173

Closed herjy closed 4 years ago

herjy commented 4 years ago

This PR adds a few features for initialising sources as discussed in issues #171 and #172.

pmelchior commented 4 years ago

I agree with the problem you seek to address, namely that the initialization of the sources could be better if it was done delayed and simultaneous, e.g. for sharing a multi-resolution detection coadd that is expensive to produce. One could also think of fitting for the SEDs of all sources once their morphologies are initialized because that's a linear solution. Other ideas are probably conceivable as well.

Here's the crux: You need to know that the source wants to be (or rather: what it can fit; what are feasible solutions) to choose an initialization that works well. This is why the initialization procedure is part of the XYZSource class implementation. I realize that you haven't changed that aspect, but you've hidden it in the initialise(...) method, which has a pretty opaque and practically unmanageable implementation.

I much prefers @fred3m's proposal of optionally feeding a detection coadd to the initialization. It's a minimal change to the current master and solves your immediate problem.

herjy commented 4 years ago

But it's already what we do! Not a lot is hidden and providing a coadd is actually possible with this update.

herjy commented 4 years ago

In the end, I'm ok to drop the initialisation function and construct the coadd to feed to the initialisations of the sources, but I have yet to see a convincing argument in favour of the current implementation. I really don't like the fact that for each source we have to provide the same set of arguments. External coadds will make it even worse.

fred3m commented 4 years ago

Actually, one last comment. Could you please change the default min_gradient to 0.1? Otherwise the stamps in blends with a lot of sources are much too big, since they can grow arbitrarily large without decreasing in flux at all. I tested using min_gradient=0.1 on galaxies with high eccentricity and galaxies with a large faint disk and (at least for ground based images) this works much better. I'd change it and push an update myself but then I'd cause merge conflicts on your end.

herjy commented 4 years ago

I get where this is coming from, but the notebooks don't do as good with 0.1. For instance here is the notebook with min_grad at 0.1 (e_rel = 1.e-7 to let it go longer).

Screen Shot 2020-05-25 at 4 43 16 PM

Actually, one last comment. Could you please change the default min_gradient to 0.1? Otherwise the stamps in blends with a lot of sources are much too big, since they can grow arbitrarily large without decreasing in flux at all. I tested using min_gradient=0.1 on galaxies with high eccentricity and galaxies with a large faint disk and (at least for ground based images) this works much better. I'd change it and push an update myself but then I'd cause merge conflicts on your end.

fred3m commented 4 years ago

Hmm, I can't tell by looking at it that it's any worse than the current master from the docs.

Are the residuals larger, and if so, is it only because they're running longer? I'm just surprised because all of the HSC images that I tested show improvement with little to no effect on the residuals.

fred3m commented 4 years ago

Wait, that's the point source tutorial, so none of those are initialized as extended sources. So this shouldn't affect them at all.

herjy commented 4 years ago

I have something very Diffrerent (min_grad at 0):

Screen Shot 2020-05-25 at 5 21 38 PM

Also, yes, two sources are Extended ones in this notebook

pmelchior commented 4 years ago

The noise level in this image is unknown and also not set from sep's background. The quality of this model is questionable in all cases.

The setting of min_gradient=0.1 re-establishes what we have done in the past. It's true that this setting is a compromise between compactness and completeness. Different cases will respond differently. Please set min_gradient=0.1, let Jarvis run the tests, and merge.

fred3m commented 4 years ago

Sigh, looks like the notebook logL is failing. I'm fine with removing this line now anyway because the new regression tests will supersede it and should be ready by next week.

herjy commented 4 years ago

Grrrr sorry, I need to find why my pytest won't run the notebook correctly