Measurements Upgrades: coordinates, bitflag, psf photometry #288

guynir42 · 2024-05-23T13:36:23Z

I've neglected to add a few things that should have been properly propagated from cutouts to measurements or calculated for the new measurements:

the coordinates should include the centroid offset for each measurement.
the badness bitflag that gets propagated from Image all the way down to Cutouts needs to also be propagated to measurements (and I've added a disqualifier cut that doesn't save Measurements that have a non zero bad flag).
PSF photometry: I've added this part using the new image's PSF (rather than the ZOGY PSF which isn't able to get sub-pixel shifted clips).
Forced photometry on cutouts: each measurement is able to give the photometry (in any aperture, or PSF) for any ra/dec that is close to the position of the cutouts. This doesn't replace needing to do forced photometry on images where there was no detection and thus no cutout.

rknop

Some questions and comments.

rknop · 2024-05-28T14:08:53Z

improc/photometry.py

+                (self.datasize - self.imsize) // 2 : (self.datasize + self.imsize) // 2,
+                (self.datasize - self.imsize) // 2 : (self.datasize + self.imsize) // 2,
+            ]
+


This does raise the issue of doing photometry in a radius that's larger than the image size. This should probably return an error, or at lest a warning.

(When did this come up?)

I've been using fairly small cutouts (25 pixels) and some of the default apertures are really big. I think we talked about changing them.

Hmm. This is pretty serious downside on doing photometry on cutouts rather than directly on the sub image. To really do it may require cutouts to be bigger than we would otherwise want them to be.

I don't really see why we need to have such large apertures.

rknop · 2024-05-28T14:10:14Z

improc/photometry.py

-        Default is False.
+        For each aperture, will measure and reposition the centroid
+        this many times before moving on to the next measurement.
+        Default is 2.


For clarity: does it do iterations measurements, updating the position after each one, and then do a final pass, for a total of iterations+1 measurements? Or does it do iterations-1 measurements, updating the posititon after each one, and then make a final measurement, for a total of iterations measurements?

It will do number of radii * iterations + a final run of number of radii

rknop · 2024-05-28T14:11:49Z

improc/photometry.py

+        if denominator == 0:
+            denominator = 1.0
+        elif abs(denominator) < 1.0:
+            denominator = 1.0 * np.sign(denominator)  # prevent division by zero and other rare cases


Why only when abs(denominator) < 1.0?

I chose an arbitrary minimum value but it can be made much smaller if you think we might see such values.

rknop · 2024-05-28T14:18:47Z

improc/photometry.py

-        photometry['moment_xx'] = cxx
-        photometry['moment_yy'] = cyy
-        photometry['moment_xy'] = cxy
+        # for each radius, do 1-3 rounds of repositioning the centroid


I'm confused about this.

It does this recentering for every radius, but unless I'm reading something wrong, it throws out the centroids for everything except the last radius. There's just a single cx variable, so it's going to get overwritten every time thorugh the loop over radii; the saving of cx and cy to best_cx and best_cy happens after the radius loop.

Why bother doing this for all of the radii if the results aren't going to be kept for anything but the last radius? (I recognize that it's not identical, because the results of previous radii are used as starting points for later radii, but I'm guessing that unless iterations=1, that's going to have a very small effect.)

The goal is to slowly lock in the correct position.
When the aperture is big, it has a good chance of having the PSF inside it's area, but it also has a lot of noise that might make it less accurate. Then a little bit smaller aperture has a good chance of still having the star but will improve the centroid position. The final (smallest) aperture gives the best centroid and we use that for actually measuring the flux and second moments.

rknop · 2024-05-28T14:29:51Z

models/measurements.py

+            # get the subtraction PSF or (if unavailable) the new image PSF
+            psf = self.cutouts.sources.image.get_psf()
+            psf_clip = psf.get_clip(x=image_pixel_x, y=image_pixel_y)
+            offset_ix = int(np.round(offset_x))


Perhaps something to worry about -- in models.psf, PSF.get_clip() has the following code:

xc = int( np.floor(x + 0.5) ) yc = int( np.floor(y + 0.5) )

Usually this should be the same as int(np.round()), but it won't when offset is exactly +0.5. (Or, for numbers bigger than ±1, when ( ( offset is even ) and ( offset > 0 ) and ( frac(offset)==0.5) ) or ( ( offset is odd ) and (offset < 0 ) and (frac(offset)==0.5) ).)

We should probably just edit PSF.get_clip() to use np.round().

OK I removed the 0.5 from get_clip.

rknop · 2024-05-28T14:53:01Z

models/objects.py

+            The sigma to use for the clipping of the measurements. Default is 3.0.
+        iterations: int, optional
+            The number of iterations to use for the clipping of the measurements. Default is 3.
+        measurement_list_kwargs: dict, optional


Thinking about this, and looking at get_measurements_list(), I'm thinking that we should probably require a prov_hash argument to get_measurements_list() (and thus here as well).

You have a default, which is as sane as any default could be. (At least, until we have some sort of version tagging table where we can tag provenances with names; at that point, we could have a 'default' tag, and then the provenances tagged that way are what would be used as default.) What I'm worried about, though, is somebody in the future looking at this code and editing it, and seeing the methods that are available, and assuming that the defaults are going to be the right thing to use unless you know otherwise. Provenances are a complicated thing, though, and it's very easy to come up with situations where the default you have isn't the right one. Given that provenances are complicated, if we make the API clear that you have to understand them well enough to do the right thing, it will force people to think about it rather than making a call that is legal as defined that will return something different from what they were expecting. (And, in such a way that they wouldn't know it was different from what they were expecting.)

I generally agree: this is a place that might surprise the user (which is bad in general). But I am also not happy to have provenance hashes as something you MUST know to use this function.
Right now it does something reasonable and can let you use it without getting into the habit of choosing provenances and copying hashes.
I don't know, I can go either way on this.

rknop · 2024-05-28T14:55:31Z

models/objects.py

+        points = SkyCoord(ra, dec, unit='deg')
+
+        ra_mean = np.nansum(ra * flux) / np.nansum(flux)
+        dec_mean = np.nansum(dec * flux) / np.nansum(flux)


What about the case where only one of (ra,dec) is NaN, and flux is not NaN? I suspect this will be rare (it may not even ever happen). But, in case it does, might be better to filter so that ra_mean doesn't include measurements where dec is NaN (and likewise for dec).

...In fact, it's bad if flux is NaN but ra is not NaN, or vice versa. Then the normalization is wrong.

Probably need something like

good = ( ~( np.isnan( ra ) | np.isnan( dec ) | np.isnan( flux ) ) ra_mean = ra[good].sum() / flux[good].sum dec_mean = dec[good].sum() / flux[good].sum

(Might also want to put in a flux > 0 filter, or maybe flux/dflux > 5..)

Yeah that works. I'm going with flux/fluxerr > 3.0, unless you feel it really should be 5.0

3.0 is fine

models/objects.py

rknop · 2024-05-28T15:08:24Z

tests/models/test_measurements.py

+    flux_small_aperture = m.get_flux_at_point(m.ra, m.dec, aperture=1)
+    flux_large_aperture = m.get_flux_at_point(m.ra, m.dec, aperture=len(m.aper_radii) - 1)
+    flux_psf = m.get_flux_at_point(m.ra, m.dec, aperture=-1)
+    assert flux_small_aperture[0] == pytest.approx(m.flux_apertures[1], abs=0.01)


These verify that a new run of get_flux_at_point returns what is expected.

However, looking at the fixtures, it looks like the values inside ptf_datastore are created using a Measurer, which (I believe-- I haven't dug through the code to verify) will have itself been done through a get_flux_at_point call.

Are there any explicit tests of get_flux_at_point itself? (That is, make sure it's doing what we expect, not that it does the same thing every time.)

The PSF flux is calculated using the same code, but the aperture flux is gotten through the photometry module.
I have some tests for that code in test_measuring and it seems to be doing the right thing, but maybe I can add something to test the PSF photometry over there.

models/base.py

models/cutouts.py

whohensee

Nothing jumped out at me besides a few questions for my own sqlalchemy/general python knowledge. I'm not entirely familiar with the exact details of the math in all of these, but since I've been lookin at these objects for the past few days I was able to mostly follow. Generally looks good to me.

guynir42 added 12 commits May 23, 2024 13:30

propagate bitflags to measurements

9789566

add disqualifier for bad flag

925e812

split tests

a62d3a2

update gitignore

81e140b

make sure measurements have bitflag

e70d823

working out the tests

a926b8b

merge main

6b870a1

improved photometry

009df66

start fixing tests

4eece8a

Merge remote-tracking branch 'upstream/main' into measurements-coords

5199058

fix tests

0854ee4

add flakiness to filtering test

91812c4

guynir42 changed the title ~~Measurements Upgrades: coordinates and bitflag~~ Measurements Upgrades: coordinates, bitflag, psf photometry May 28, 2024

guynir42 marked this pull request as ready for review May 28, 2024 12:08

guynir42 requested review from rknop and whohensee May 28, 2024 12:08

rknop reviewed May 28, 2024

View reviewed changes

whohensee reviewed May 28, 2024

View reviewed changes

models/base.py Show resolved Hide resolved

whohensee reviewed May 28, 2024

View reviewed changes

models/cutouts.py Outdated Show resolved Hide resolved

whohensee reviewed May 28, 2024

View reviewed changes

guynir42 added 4 commits May 29, 2024 09:59

address reviewer comments

8ebc91e

fix tests

7e9c382

fix more tests

448e225

fix test

ec360f2

guynir42 requested a review from rknop May 30, 2024 12:22

rknop approved these changes May 30, 2024

View reviewed changes

rknop merged commit efab173 into c3-time-domain:main May 30, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Measurements Upgrades: coordinates, bitflag, psf photometry #288

Measurements Upgrades: coordinates, bitflag, psf photometry #288

guynir42 commented May 23, 2024 •

edited

Loading

rknop left a comment

rknop May 28, 2024

guynir42 May 28, 2024

rknop May 28, 2024

guynir42 May 29, 2024

rknop May 28, 2024

guynir42 May 28, 2024 •

edited

Loading

rknop May 28, 2024

guynir42 May 28, 2024

rknop May 28, 2024

guynir42 May 28, 2024

rknop May 28, 2024

guynir42 May 28, 2024

rknop May 28, 2024

guynir42 May 29, 2024

rknop May 28, 2024

rknop May 28, 2024

guynir42 May 29, 2024

rknop May 29, 2024

rknop May 28, 2024

guynir42 May 29, 2024

whohensee left a comment

Measurements Upgrades: coordinates, bitflag, psf photometry #288

Measurements Upgrades: coordinates, bitflag, psf photometry #288

Conversation

guynir42 commented May 23, 2024 • edited Loading

rknop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guynir42 May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whohensee left a comment

Choose a reason for hiding this comment

guynir42 commented May 23, 2024 •

edited

Loading

guynir42 May 28, 2024 •

edited

Loading