FEAT: add argument to specify calibration correction direction

[ColmTalbot]

This PR aims to add sufficient flexibility in reading calibration data products to make sure that we correctly map these to the templates in the likelihood. There are probably additional documentation changes that we will want.

[mj-will]

We had some discussion about this on the last dev call but not many of us attended. So I'd encourage people to comment.

@asb5468 could you add a comment with what you said on the call?

[farr]

@ColmTalbot FWIW, I like this change, particularly the description and choices in the keyword argument correction='data' or correction='template'. One thing I noticed which is subtle (but, as written, correct!) is in the reading of the calibration uncertainties in phase and amplitude: currently the code assumes the "ordering" of the quantiles given in the calibration file and performs subtraction to determine the sigma parameters in the calibration envelope (ordering reversed for data calibration where you are taking the inverse):

        if correction.lower() == "data":
            amplitude_median = 1 / calibration_data[1] - 1
            phase_median = -calibration_data[2]
            amplitude_sigma = (1 / calibration_data[3] - 1 / calibration_data[5]) / 2
            phase_sigma = (calibration_data[6] - calibration_data[4]) / 2
        else:
            amplitude_median = calibration_data[1] - 1
            phase_median = calibration_data[2]
            amplitude_sigma = (calibration_data[5] - calibration_data[3]) / 2
            phase_sigma = (calibration_data[6] - calibration_data[4]) / 2

I might throw an np.abs(...) around those, just to be robust against changing the ordering (ultimately, it would be nice to have the columns named, instead of indexed, so you can just do (calibration_data['amplitude_q84'] - calibration_data['amplitude_q16'])/2 and don't have to rely on opaque column numbers).

[transientlunatic]

This lgtm.

[asb5468]

This is a minor point, but can the new parameter be called correction_type rather than correction? The latter to me seems like an optional boolean variable, like whether to implement a higher-order correction.

[cplb]

I'd suggest either (i) we make correction='data' instead of None, or (ii) change the text here to say something like traditional or usual rather than default. I suspect (ii) is better, as we want to avoid cases where people don't know which one they are doing.

[ColmTalbot]

Is the issue here that None is mapped internally to data ad so data technically isn't the default value? I'm not really concerned about that as practically the default is data.

[cplb]

Same comment here as on L37 https://github.com/bilby-dev/bilby/pull/47/files#r1795826808

[adivijaykumar]

Broadly LGTM! I have left a few comments; I also agree with suggestions raised by Sylvia and Will.

[jacobgolomb]

Agree with correction -> correction_type, but also I think the default in the signature should be correction_type='data' and there should just always be a logger message saying saying what it is doing.

[mj-will]

There are a few comments about whether the default should be None or 'data'. FWIW, I'm in favour of keeping it as None for now with a warning, as it makes it very clear that you should be setting it.

In future, I'd be in favour of going a step further and either making it a keyword argument without a default, meaning the function cannot be called without specifying the type of correction being used. This would break any codes which do not set it and avoid potential issues due to assumed behaviour/ignoring warnings.

[ColmTalbot]

(ultimately, it would be nice to have the columns named, instead of indexed, so you can just do (calibration_data['amplitude_q84'] - calibration_data['amplitude_q16'])/2 and don't have to rely on opaque column numbers).

@farr I agree in principle, in practice, parsing the header of these files is sufficiently difficult that I'd like to at least leave that to a follow up.

[ColmTalbot]

Thanks everyone for the comments! I think I've addressed most of the questions here. The major outstanding point is essentially, should the code always emit a message when reading in the calibration envelopes? Currently, I've set this as a debug statement, so people can see it if they like, but it won't always be printed by default. My main argument for this is that if people have specified the method, then telling them which method is being used is just noise.

As a reminder, if users don't specify an option they will get a warning, and probably an error in future versions.

Can people react thumbs up for keeping as debug or thumbs down for making this a logger.info?

[ColmTalbot]

@hoyc1 @SimonStevenson please can you look at this PR if you have a chance?

[ColmTalbot]

This is now merged, thanks everyone for the feedback!