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

⚡ tweaking PSFmachine to play with the K2 implementation #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

christinahedges
Copy link
Contributor

@christinahedges christinahedges commented Feb 3, 2022

I'm taking a look at PSFmachine for ruprecht, and making a few tweaks. I'm opening this issue to discuss these tweaks.

Here's a rough log of changes:

  1. Changed the _update_source_mask... function so that it more aggressively clips out the faint edges of the PSF
  2. Messed a little with some of the sane defaults for the masking function
  3. Added a slightly more robust error catch for the "breaks" in time binning,
  4. Get centroid takes a frame index so it will only calculate the centroid of the current frame unless "mean" is passed...(this might not be advisable)
  5. _get_source_mask now puts the centroid to the centroid of the frame_index, making sure that per cadence PSFs are always centered at zero.
  6. build_frame_shape_model recalculates the source mask at every step

The last point makes sure that the PSF is always centered at 0,0 when we fit it. This is important, as our model is only valid if the PSF it's modeling is centered.

However, the way it is currently set up, this becomes quite intractable for large datasets...!

@christinahedges christinahedges changed the title [WIP] ⚡ tweaking PSFmachine to play with the K2 implementation ⚡ tweaking PSFmachine to play with the K2 implementation Mar 18, 2022
Comment on lines +326 to +329
upper_radius_limit=36.0,
lower_radius_limit=4.5,
upper_flux_limit=2e5,
lower_flux_limit=100,
lower_flux_limit=40,
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes the rough mask more generous, good when scene motion is considerable, and allows for fainter pixels

Copy link
Contributor

@jorgemarpa jorgemarpa left a comment

Choose a reason for hiding this comment

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

I like the small tweaks you add to the API. Only a few of them change the default values, the rest only makes the API more robust. I don't think the first ones will make big changes for Kepler data.

We need to update the tutorials to reflect the new keyword arguments we are introducing here. I saw the tutorials in christinatweaks branch aren't updated to the last version in master.
Also, the change to superstamp.py will make the computing times in the SSMachine tutorials a bit longer.

I will run the Tutorials as a way to check things are consistent with the master branch, I'll report back when it's done.

# mask out non finite values and background pixels
k = (np.isfinite(wgts)) & (
self.uncontaminated_source_mask.multiply(self.flux[t]).data > 100
self.uncontaminated_source_mask.multiply(self.flux[t]).data > 10
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this value should be more generous.
Let's add a comment that this value works well for Kepler/K2 background-subtracted pixels.

correct_centroid_offset=True,
plot=False,
frame_index="mean",
Copy link
Contributor

Choose a reason for hiding this comment

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

add docstring for this argument.
Using main keeps the original behavior, right?

@@ -1048,16 +1083,18 @@ def plot_time_model(self):
return fig

def build_shape_model(
self, plot=False, flux_cut_off=1, frame_index="mean", **kwargs
self, plot=False, flux_atol=1, flux_rtol=0.001, frame_index="mean", **kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these new arguments are better than the ones we had before.

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.

2 participants