Fix multiresolution metrics #174
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
It should work right now, though it isn't very pretty yet ; you can see the results in the SCARLET notebooks. @ismael-mendoza you are free to take a look ; I will try to make it more maintainable. |
There was a problem hiding this comment.
Hi @thuiop , thanks for your work on this issue! I added a few minor comments, please feel free to ignore any are solely because this might still be partially in WiP.
It might also be good if you gave us a walkthrough in the next meeting because I'm not sure I understand all the changes.
Quick question, scarlet has a MR feature for deblending right? Did you leverage that in the notebook or did you just run it separately for each survey (anyway is fine I was just curious)
Yes, I used the multiresolution feature here. |
|
This should be somewhat mergeable now. |
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
==========================================
+ Coverage 84.12% 86.34% +2.21%
==========================================
Files 13 13
Lines 1424 1509 +85
==========================================
+ Hits 1198 1303 +105
+ Misses 226 206 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@ismael-mendoza To give a clearer overview of what happens on the user side :
|
There was a problem hiding this comment.
Thanks @thuiop , looks great! Thanks a lot for your work on this. Feel free to merge after resolving my small comments below.
|
|
||
| # set detection threshold to 5 times std of image | ||
| threshold = 5 * np.std(coadd) | ||
| coordinates = peak_local_max(coadd, min_distance=2, threshold_abs=threshold) | ||
|
|
||
| # construct catalog from measurement. | ||
| catalog = astropy.table.Table() | ||
| catalog["x_peak"] = coordinates[:, 1] | ||
| catalog["y_peak"] = coordinates[:, 0] | ||
| catalog["ra"], catalog["dec"] = wcs.pixel_to_world_values(coordinates[:, 1], coordinates[:, 0]) |
There was a problem hiding this comment.
I see that you now assume that 'ra', 'dec' are always returned (even if MR feature is not used). That makes sense, since these requires an additional step from the user maybe we should mention this somewhere in the documentation (how to access wcs and use it to convert your pixel coordinates)?
There was a problem hiding this comment.
Yes, this will be done in the following PR.
| f"{out[key].shape[-3:]} vs {batch['blend_images'].shape[-3:]}" | ||
| if len(surveys) == 1: | ||
| if not isinstance(out[key], np.ndarray): | ||
| raise TypeError( |
There was a problem hiding this comment.
Some of the error cases are not covered by tests (see below for codecov messages). Do you mind updating the tests for coverage? If you prefer to do it later (or for me to do it) feel free to just open an issue about it so we don't forget.
There was a problem hiding this comment.
I feel like this is not an urgent issue as long as we keep the coverage up, so I will open an issue for the time being.
Measure and metrics are currently not compatible with multiresolution, which I try to fix in this PR (closes #160 and #166).
To do this, measure functions should return ra and dec coordinates instead of pixels which is easy to do since we have the WCS. The deblended images and segmentations should also be returned as a dictionnary indexed by the survey names. Then, in the metrics generator, the compute_metrics function is called for each survey (we recover pixel coordinates at this stage using the WCS).