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

Started a PRF update sketch #1365

Draft
wants to merge 71 commits into
base: main
Choose a base branch
from
Draft

Conversation

Nschanche
Copy link
Collaborator

@Nschanche Nschanche commented Sep 11, 2023

This PR will add TESS support to the PRF module. It will also substantially change the structure and functionality of the PRF module.

This PR is an update to prfmodel.py.
In addition to adding functionality for TESS, this PR makes a number of functional and structural changes.

Changes from original

  • Adds functionality for TESS
  • TESS PRF files are modified to '0-out' pixels that do not contribute to the PRF
  • Kepler and TESS files are stored in the lightkurve file structure (ie not downloaded from the internet)
  • The Interpolated PRF is cropped so that the flux contribution cannot extend past the PRF file size.
  • The structure of the call is changed (specifics of which I'm still deciding on)

Use cases and functionality I am considering:

  • "I want to make an aperture for a TESSCut TPF"

    • tpf.prf.estimate_aperture()
  • "I want to build a PRF model to simulate a real TPF"

    • call PRF(tpf). This gets channel/cam/ccd, col/row from tpf header
    • Each TargetPixelFile has a prf attribute which is a callable PRF instance tpf.prf. Users can e.g. do tpf.prf.estimate_aperture() or tpf.prf.estimate_aperture(completeness=0.9) or tpf.prf.model(position)
    • obtain a table of star positions (either in coords, pixel coords, magnitudes or flux)
    • return a np.ndarray of values that can be multiplied by flux summed to create an image.
  • "I want to just see what the PRF looks like at a given location"

    • call PRF(channel/cam/cdd, row, col OR ra, dec)
    • lk.prf.TessPRF(camera=1, ccd=1).plot(loc)
    • lk.prf.TessPRF(camera=1, ccd=1).model(loc)
    • should return a PRF the size of the engineering files (11x11 pixels I believe) by default

New Workflow:

  • Instantiate PRF with only the channel (Kepler) or camera/ccd (TESS)
  • Call a 'model' function with the column, row, flux, center_col, center_row, flux, scale_col, scale_row, rotation_angle

@christinahedges
Copy link
Collaborator

@Nschanche here are some quick tips;

  1. Put your GREAT comments in the first post on the PR, so people can easily see (myself included)
  2. Install and use a linter (I like black but not everyone does)



# Just sketching out a few models right now
def class PRF(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def class PRF(ABC):
class PRF(ABC):

Copy link
Collaborator

Choose a reason for hiding this comment

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

You want an init function here so you can describe how you're expecting this class to be initialized. You could end up with a system that looks something like this

class PRF(object):
    def __init__(self, fname, pixel_size):
        # load PSF file
        # initialize object

class KeplerPRF(PRF):
    def __init__(self, channel, shape):
        # Some code to check it is a Kepler TPF
        super().__init__(fname=fname, pixel_size=pixel_size)

class TessPRF():
    ....

# Instantiate PRF with only the channel (Kepler) or camera/ccd (TESS)
# Call a 'model' function with the column, row, flux, center_col, center_row, flux, scale_col, scale_row, rotation_angle


Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure to import ABC

# Just sketching out a few models right now
def class PRF(ABC):

def estimate_aperture(tpf):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def estimate_aperture(tpf):
def estimate_aperture(tpf: lk.TargetPixelFile): -> npt.ArrayLike

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is going to take in more arguments, you should think about some you expect.

You're probably going to need functions to estimate

  • Completeness
  • Contamination
    (you can call them this in lightkurve)

Which we also know as FLFRSAP and CROWDSAP.

Your function here might take in a minimum value for e.g. completeness and a maximum allowed value for contamination perhaps?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might want either helper functions or exposed functions to make apertures that aren't the "best" aperture, that take in the keyword arguments above, and then estimate_pipeline_aperture would give you the best one


def estimate_aperture(tpf):
# Given a tpf, build a prf model and estimate the best aperture
def create_simple_aperture(pix_c, pix_r, ra, dec, flux, completeness)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You here need to change the api so it can take in either RA/Dec (maybe use a SkyCoord object?) or a pixel coordinate.

def create_simple_aperture(self, coord:Union[tuple, SkyCoord], completeness: float = 0.9): -> npt.ArrayLike

# Given a tpf, build a prf model and estimate the best aperture
def create_simple_aperture(pix_c, pix_r, ra, dec, flux, completeness)
# Based on completeness requirement, create an aperture
# This wouldn't worry about contamination I think
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this doesn't use contamination we don't need flux to be input so you can take that out

def create_simple_aperture(pix_c, pix_r, ra, dec, flux, completeness)
# Based on completeness requirement, create an aperture
# This wouldn't worry about contamination I think
def model(self,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I anticipate you will need a function that can make a PSF model on a higher resolution grid in order to estimate the completeness or contamination. You might have 5x the pixel size as a sane default

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this function, think about having an oversample keyword which when set to 1 returns the TPF grid spacing model and when set to e.g. 5 returns a 5x the resolution of the TPF.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can keep it as scaling each dimension but i don't think it's as useful.

You will need to be able to pass the pixel size I imagine, this will have to work for Kepler and TESS

# KEPLER PRF
#############################################################
#############################################################
class KeplerPRF(PRF):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent breaking these up.

def __init__(self, channel, shape):
self.channel = channel
self.shape = shape

Copy link
Collaborator

Choose a reason for hiding this comment

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

Inside PRF.py

@staticmethod
def from_tpf(self, tpf):
    ```Creates a PRF object using a TPF```
    # Add some error checks that it's a Kepler TPF here
    return PRF(tpf.channel, tpf.shape)

Inside TPF.py

def prf(self):
      """Returns a PRF object"""
      if self.mission.lower == 'kepler':
          return PRF.from_tpf(self)          
...
PRF.from_TPF(tpf)
tpf.prf # -> PRF object

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason I'm saying prf not PRF is because we have wcs not WCS as an attribute and this is quite similar. Happy to debate this one

class KeplerPRF(PRF):
"""A KeplerPRF class"""
# I want the option to either give it a tpf and it reads channel/shape OR provide that info
def __init__(self, channel, shape):
Copy link
Collaborator

Choose a reason for hiding this comment

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

you also need pixel size I imagine

def __repr__(self):
return "I'm a Kepler PRF"

def __call__(self, center_col, center_row, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's expose these keywords so we can be very explicit, there aren't that many keywords

@christinahedges
Copy link
Collaborator

In the workflow we've talked about you need to be careful that you aren't constantly re-reading in that PRF file if you don't need to. You can use lru_cache potentially to help with this, if they are small it might be ok to load them in every time, but some people might want to hammer this function.

@christinahedges christinahedges added the 💪 WIP This is a work in progress! label Sep 19, 2023
@Nschanche
Copy link
Collaborator Author

You can check out a demo of the current functionality here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 WIP This is a work in progress!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants