Feature - calibrated argument #53
Conversation
There was a problem hiding this comment.
Thanks for opening a PR for this useful feature!
I left some comments on the code. The only other minor suggestion would be to show an example of using the new calibrated=False feature in one of the tutorial notebooks. For example this one.
| @@ -155,7 +155,7 @@ def __call__(self, q, M=None, dist=None, phi_ref=None,\ | |||
| times --- array of times at which surrogate is to be evaluated | |||
| units --- units (mks or dimensionless) of input array of time samples | |||
| singlemode_call --- Never set this by hand, will be False if this routine is called by the multimode evaluator | |||
|
|
|||
| calibrated --- If set to false, the calibration coefficients alpha and beta are set to 1 | |||
There was a problem hiding this comment.
calibration is only applicable for the models EMRISur1dq1e4 and BHPTNRSur1dq1e4. Can you please add a short remark about this in the docstring?
| @@ -183,7 +183,7 @@ def __call__(self, q, M=None, dist=None, phi_ref=None,\ | |||
|
|
|||
| if (M is not None) and (dist is not None) and (times is not None) and (units != 'mks'): | |||
| raise ValueError('passing values of M, dist, and times suggest mks units should be used!') | |||
|
|
|||
There was a problem hiding this comment.
At this part of the code there's a bunch of checks to make sure the user passes something reasonable.
I think we should check that if the model is not one of EMRISur1dq1e4 or BHPTNRSur1dq1e4 and calibration=False an error is raised. Because this would suggest the user is trying to override the calibration default for a model that doesn't support this parameter in the first place.
It might also be safer to assign calibration=None if the model is not one of EMRISur1dq1e4 or BHPTNRSur1dq1e4. This is just another safeguard that the code doesn't somehow depend on the value of calibration when it shouldn't
| @@ -196,18 +196,21 @@ def __call__(self, q, M=None, dist=None, phi_ref=None,\ | |||
| # at a given value of parameter space. These calibration parameters will scale the | |||
| # time and amplitude. Currently, these models use calibration: | |||
| # BHPTNRSur1dq1e4, EMRISur1dq1e4 | |||
| if(self.surrogateID == 'BHPTNRSur1dq1e4'): | |||
| if not calibrated: | |||
There was a problem hiding this comment.
perhaps nest this entire if-statement under if(self.surrogateID in ['EMRISur1dq1e4','BHPTNRSur1dq1e4']):. So it could be something like
if(self.surrogateID in ['EMRISur1dq1e4','BHPTNRSur1dq1e4']):
# CODE YOU ALREADY HAVE
else:
alpha_nr_calibration, beta_nr_calibration = None, NoneWhich is again to be extra careful about not defining calibration parameter values for models that don't use them
| @@ -1059,7 +1056,7 @@ def __init__(self, path, deg=3, ell_m=None, excluded='DEFAULT', use_orbital_plan | |||
| def __call__(self, q, M=None, dist=None, theta=None,phi=None, | |||
| z_rot=None, f_low=None, times=None, | |||
| units='dimensionless', | |||
| ell=None, m=None, mode_sum=True,fake_neg_modes=True): | |||
| ell=None, m=None, mode_sum=True,fake_neg_modes=True, calibrated=True): | |||
There was a problem hiding this comment.
same comments as above about the docstring discussing calibrated
This feature adds 'calibrated' (boolean) argument to the EvaluateSurrogate call. This argument turns on/off the calibration in the surrogate being evaluated.