-
Notifications
You must be signed in to change notification settings - Fork 3
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
deprecated transiting_code, legacy_pspec.py and C_empirical() #95
Conversation
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.
Looks good. Only very minor comments about this actual PR, but it raises some interesting conceptual issues that we should address soon.
hera_pspec/pspecdata.py
Outdated
@@ -431,10 +402,9 @@ def set_C(self, cov): | |||
self.clear_cov_cache(cov.keys()) | |||
for key in cov: self._C[key] = cov[key] | |||
|
|||
def C_empirical(self, key): | |||
def C_model(self, key, mtype='empirical'): |
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 model=
instead of mtype=
?
I'm a bit worried about consistency here by the way. What happens if the user calls this with one model type, and then tries to use another model type later on? Is the cache dropped and recomputed in this case? Or should we store one cache per model type?
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.
It's also unclear to me how we will support more sophisticated covariance matrix methods in future. Should we be getting the user to use set_C
manually? Or should we attach some wrapper object, similar to PSpecBeam
, that handles all of the covmat stuff?
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.
Yeah that's a good point about the cache. As it stands there is only one slot for a covariance matrix, regardless of its type, so the most recently "set" covmat is the one it will pick up on. Perhaps when we re-haul inverse covariance weighting we should take a closer look at how we implement the storing of various covariance matrix types.
updated C_model to account for model in key
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.
Looks good to me!
This deprecates the
transiting_code
folder that has been hanging around during the graduation of OQE. This is now completed and therefore is no longer necessary to keep around.This also deprecates the
legacy_pspec.py
module that was put in place in case other users wanted to interface with legacy techniques throughhera_pspec
. This was never really used (according to my knowledge), so we are deprecating it to clean things up.Lastly, we found that within
pspecdata
, theC_empirical()
andC()
methods were doing the same thing of calling theutils.cov()
function to calculate empirical covariances. We are deprecatingC()
to avoid confusion and ambiguity as to what function is called to calculate an empirical covariance.Addresses #93