-
Notifications
You must be signed in to change notification settings - Fork 5
✨ Developing PerturbationMatrix class #59
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
Conversation
|
Roughly how to use: If P = PerturbationMatrix3d(time=time, dx=dx, dy=dy)
P.fit(flux, flux_err)
model = P.model()
|
jorgemarpa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I think the basic functionalities are implemented here.
Something that will be good to have is a pixel_mask argument in PerturbationMatrix3D so we can create matrix with the source_mask (for fitting) pixels and the uncontaminated_source_mask (for building) in one perturbation object.
I did a couple of "performance" test with TPF data and turned out that the self.model method runs really slow, about 1.7 sec/iter. We need to find why this is so slow.
|
I added a PCA method so you can do something like: y = [whitened_timeseries, whitened_timeseries]
p = PerturbationMatrix(...)
p.pca(y, ncomponents=10)
p.fit(y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christinahedges I found the bug with the inconsistent shape when doing self.model(). The problem is when using PerturbationMatrix through PerturbationMatrix3D that some attributes from the latter are not updated to reflect the addition of PCA components in self.vectors. See my comments for more details.
|
Any vectors that aren't the time polynomial are now set to zero mean in each "segment" during |
Co-authored-by: Jorge Martínez-Palomera <[email protected]>
jorgemarpa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I tested a handful of component combinations and other parameters to check the matrices look Ok.
It fulfills what we always thought about the perturbation matrix API.
I just added a couple of comments regarding docstrings. Besides those, I think this is ready to be merged.
|
|
||
| return func | ||
|
|
||
| def pca(self, y, ncomponents=5, smooth_time_scale=0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be consistent with the rest
| def pca(self, y, ncomponents=5, smooth_time_scale=0): | |
| def pca(self, y, ncomponents=3, smooth_time_scale=0): |
src/psfmachine/perturbation.py
Outdated
| Will add two time scales of principal components, definied by `long_time_scale` | ||
| and `med_time_scale`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Will add two time scales of principal components, definied by `long_time_scale` | |
| and `med_time_scale`. |
src/psfmachine/perturbation.py
Outdated
| long_time_scale: float | ||
| The time scale where variability is considered "long" term. Should be | ||
| in the same units as `self.time`. | ||
| med_time_scale: float | ||
| The time scale where variability is considered "medium" term. Should be | ||
| in the same units as `self.time`. Variability longer than `long_time_scale`, | ||
| or shorter than `med_time_scale`, will be removed before building components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the scale of smooth_time_scale?
| long_time_scale: float | |
| The time scale where variability is considered "long" term. Should be | |
| in the same units as `self.time`. | |
| med_time_scale: float | |
| The time scale where variability is considered "medium" term. Should be | |
| in the same units as `self.time`. Variability longer than `long_time_scale`, | |
| or shorter than `med_time_scale`, will be removed before building components | |
| smooth_time_scale: float | |
| Amount to smooth the components, using a spline in time. | |
| If 0, the components will not be smoothed. |
| # def _clean_vectors(self): | ||
| # """Remove time polynomial from other vectors""" | ||
| # nvec = self.poly_order + 1 | ||
| # if self.focus: | ||
| # nvec += 1 | ||
| # if self.segments: | ||
| # s = nvec * (len(self.breaks) + 1) | ||
| # else: | ||
| # s = nvec | ||
| # | ||
| # if s != self.vectors.shape[1]: | ||
| # X = self.vectors[:, :s] | ||
| # w = np.linalg.solve(X.T.dot(X), X.T.dot(self.vectors[:, s:])) | ||
| # self.vectors[:, s:] -= X.dot(w) | ||
| # # Each segment has mean zero | ||
| # self.vectors[:, s:] -= np.asarray( | ||
| # [v[v != 0].mean() * (v != 0) for v in self.vectors[:, s:].T] | ||
| # ).T | ||
| # return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can still keep this function in case we want to remove the temporal trend for other components. But it isn't necessary with the current API.
This PR is for developing a new PerturbationMatrix class which will ultimately factor our a lot of the headaches we're having inside machine
To Do