Refactor API to remove in-place centroids, and update FFIMachine [do-NOT-merge] #54

christinahedges · 2022-03-18T21:51:50Z

This PR refactors a lot of our API to make sure we have no in-place centroids. Here are some top changes:

We now have a rough_mask, which is our first pass at the masking
The source_mask is now made in a slightly more robust and clear way
FFIMachine now loads images in a more memory efficient way
We're no longer using that rtol to clip out faint parts of the PSF. We just use the atol, i.e. where the PSF has counts greater than some value.
For FFIMachine, we don't remove pixels that are saturated or "bad", we just make sure that in all the masks (rough_mask, source_mask, uncontaminated_source_mask) these are False.
I added a different background correction that uses splines to fit a smooth background model. This works well for TESS...I need to test it for Kepler FFIs
Makes FFIs work without removing pixels

This PR supersedes #48

To Do

Check this runs for Kepler FFIs!

…es/psfmachine into tess

christinahedges · 2022-03-18T21:59:48Z

This explains what each of the three masks are:

christinahedges · 2022-03-18T22:12:13Z

There's a notebook here which shows how to use this PR for getting the TESS photometry out...

jorgemarpa

In the near future, we need to merge the changes from #60 including the perturbation API. I think after that the only 'big' changes to machine.py are the way to compute the source_mask and centroids.

jorgemarpa · 2022-04-19T22:22:05Z

src/psfmachine/machine.py

+    @property
+    def dx(self):
+        """Delta RA, corrected for centroid shift"""
+        if not sparse.issparse(self.dra):
+            return self.dra - self.ra_centroid.value
+        else:
+            ra_offset = sparse.csr_matrix(
+                (
+                    np.repeat(
+                        self.ra_centroid.value,
+                        self.dra.data.shape,
+                    ),
+                    (self.dra.nonzero()),
+                ),
+                shape=self.dra.shape,
+                dtype=float,
+            )
+            return self.dra - ra_offset


This is the core change to avoid in-place operation. Does this adds overhead when calling self.dx, I don't think it'd matter much though, only for the case of large sparse data.

jorgemarpa · 2022-04-19T22:29:45Z

src/psfmachine/machine.py

+    def _update_delta_arrays(self, frame_indices="mean"):
+        if self.nsources * self.npixels < 1e7:
+            self._update_delta_numpy_arrays()
+        else:
+            self._update_delta_sparse_arrays()


I think this should be renamed to just _create_delta_*_arrays() (or just _delta_*_arrays()) there's no "update" happening there.
The frame_indices="mean" argument I think is unnecessary now.

jorgemarpa · 2022-04-19T22:30:33Z

src/psfmachine/machine.py

+        if frame_indices == "mean":
+            frame_indices = np.where(self.time_mask)[0]


This isn't doing anything in the function.

jorgemarpa · 2022-04-19T22:38:11Z

src/psfmachine/machine.py

-        plot=False,
-    ):
-        """Find the pixel mask that identifies pixels with contributions from ANY NUMBER of Sources
+    def _get_source_mask(self, source_flux_limit=1):


this new version of _get_source_mask looks "simpler" than the original one, I mean, with less tunable params, which I like.

Is iterating 2 times good to converge into a solid source_mask?

A diagnostic plot for this one would be great, something to check we are capturing well the f, r dependency.

jorgemarpa · 2022-04-19T22:50:20Z

src/psfmachine/machine.py

-            # mask out non finite values and background pixels
-            k = (np.isfinite(wgts)) & (
-                self.uncontaminated_source_mask.multiply(self.flux[t]).data > 100
+    def _get_centroid(self, plot=False):


we need short documentation here

Now, this method computes a single centroid value for all frames. Do we want to also have centroids in each frame?

jorgemarpa · 2022-04-19T23:40:30Z

src/psfmachine/ffi.py

        return

-    def _remove_background(self, mask=None):
+    def _remove_background(self, mask=None, pixel_knot_spacing=10):


we can get rid of photutils dependency

jorgemarpa · 2022-04-19T23:43:50Z

src/psfmachine/ffi.py

+        thumb = np.min(self.flux, axis=0).reshape(self.image_shape)
+        gthumb = np.hypot(*np.gradient(thumb))
+        mask = (
+            ~sigma_clip(
+                np.ma.masked_array(gthumb, gthumb > 500),
+                sigma=3,
+                cenfunc=lambda x, axis: 0,
+            ).mask
+        ).ravel()
+        self._remove_background(mask=mask)


I think this can happens after super.__init__ now.

jorgemarpa · 2022-04-19T23:50:17Z

src/psfmachine/ffi.py

+        pixel_mask = self.non_sat_pixel_mask & self.non_bright_source_mask
+        self.rough_mask = self.rough_mask.multiply(pixel_mask).tocsr()
+        self.rough_mask.eliminate_zeros()
+        self.source_mask = self.source_mask.multiply(pixel_mask).tocsr()
+        self.source_mask.eliminate_zeros()
+        self.uncontaminated_source_mask = self.uncontaminated_source_mask.multiply(
+            pixel_mask
+        ).tocsr()
+        self.uncontaminated_source_mask.eliminate_zeros()


this is great, never thought on including the sat/bright pixel mask into the machine mask, this way we can keep all original pixels and do nice image plots
😃

jorgemarpa · 2022-04-19T23:56:51Z

src/psfmachine/utils.py



-def _combine_A(A, poscorr=None, time=None):
+def _combine_A(A, time, poscorr=None):


this one will disappear after merging with perturbation API

jorgemarpa · 2022-04-19T23:58:46Z

src/psfmachine/utils.py

    )
+
+
+def _find_uncontaminated_pixels(mask):


I'm not convinced this one belongs here, it used t be a hidden method of machine.py

Extracted out of #54, this is just a slightly more robust version of this part of the code.

jorgemarpa · 2022-12-13T03:40:10Z

I isolate the efficient FFI changes and open a new PR #71. We'll keep this PR open for future reference when including the in-place operations and new source mask methods.

christinahedges added 6 commits February 3, 2022 13:30

🎨 removing inplace centroids

d310765

🎨 working on centroids

d87fdbd

tweaks for tess

7b3d1a6

Merge branch 'noinplacecentroids' of https://github.com/christinahedg…

1beb237

…es/psfmachine into tess

Merge branch 'noinplacecentroids' of https://github.com/christinahedg…

c7b9199

…es/psfmachine into tess

♻️ cleaning up

bfec537

christinahedges mentioned this pull request Mar 18, 2022

[WIP] 🎨 removing inplace centroids #48

Closed

christinahedges added 4 commits March 18, 2022 15:22

♻️ flake8

ca48f45

♻️ fixing SSMachine

0bfeab0

✅ making them tests work

d5d271f

✅ flake8

1aacdf4

christinahedges requested a review from jorgemarpa April 18, 2022 21:33

jorgemarpa reviewed Apr 20, 2022

View reviewed changes

christinahedges added a commit that referenced this pull request Dec 8, 2022

Update spline to be slightly more robust

3e9c499

Extracted out of #54, this is just a slightly more robust version of this part of the code.

christinahedges mentioned this pull request Dec 8, 2022

Update spline to be slightly more robust #69

Merged

jorgemarpa changed the title ~~Refactor API to remove in-place centroids, and update FFIMachine~~ Refactor API to remove in-place centroids, and update FFIMachine [DO-NOT-MERGE] Dec 13, 2022

jorgemarpa changed the title ~~Refactor API to remove in-place centroids, and update FFIMachine [DO-NOT-MERGE]~~ Refactor API to remove in-place centroids, and update FFIMachine [do-NOT-merge] Dec 13, 2022

		if frame_indices == "mean":
		frame_indices = np.where(self.time_mask)[0]



		def _combine_A(A, poscorr=None, time=None):
		def _combine_A(A, time, poscorr=None):

Refactor API to remove in-place centroids, and update FFIMachine [do-NOT-merge] #54

Are you sure you want to change the base?

Refactor API to remove in-place centroids, and update FFIMachine [do-NOT-merge] #54

Uh oh!

Conversation

christinahedges commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christinahedges commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christinahedges commented Mar 18, 2022

Uh oh!

jorgemarpa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgemarpa commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

christinahedges commented Mar 18, 2022 •

edited

Loading

christinahedges commented Mar 18, 2022 •

edited

Loading

jorgemarpa commented Dec 13, 2022 •

edited

Loading