Tidy up of inject signal

• Changes the warning to an Error
• Adds method to InterferometerStrainData to check if a time is within the span
• Clean up logging of warning if the geocent_time is within the data span
• Print infomation about the injection to the terminal

Closes #115 (closed)

Closes #116 (closed)

mentioned in issue #115 (closed)

@colm.talbot I'm sorry to keep throwing work at you today.

This fixes #115 (closed) and also just does some minor clean up stuff.

added 1 commit

• 68c51853 - Fix tests for new raise ValueError

Compare with previous version

added 1 commit

• 594b4bae - Fix spacing of injection output

Compare with previous version

marked as a Work In Progress

changed the description

assigned to @gregory.ashton

added 2 commits

• a378ba9b - Replace injection_polarizations with waveform_generator in inject signal
• 2ab4bddd - Track changes to inject_signal in a378ba9b to the examples

Compare with previous version

unmarked as a Work In Progress

marked as a Work In Progress

changed the description

unmarked as a Work In Progress

added 1 commit

• 4c3a622b - Fixes to get the tests to pass

Compare with previous version

For the record, the arguments against passing the waveform generator are:

• This breaks if you try to inject into multiple detectors using e.g., mass_1 and mass_ratio for the injection parameters. It succeeds for the first interferometer but then these parameters aren't known for the next interferometers.
• It may be the case that the signal you're injecting takes a huge amount of time to generate, in this case you only want to do it once.

I think we can do this in a slick way using an InterferometerSet, in that case you can generalise the inject_signal method in a way which only requires a single waveform evaluation.

added 2 commits

• e11ebac1 - Revert "Track changes to inject_signal in a378ba9b to the examples"
• 2cccc662 - Adds back in injection_polarization argument

Compare with previous version

@colm.talbot Okay I've reverted to provide backward compatibility. While I appreciate the argument on grounds on it taking a long time to generate [1], but the first argument I don't understand. If we have buggy behaviour, we can surely fix it?

I tried to reproduce it by injecting with mass_1 and mass_ratio and got a ValueError that mass_2 was None (this was when injected using the polarization directly) so I don't even think you can do that at the moment. When injecting with the normal parameters everything worked fine in both cases (at least it seemed to sample okay).

I think the argument against having the polarization directly is that the user could pass in a polarization injected with one set of parameters, along with another (totally different) set of parameters (by accident). Then the results might look okay, but would be subtlety wrong and it might be difficult to track that error.

[1] I guess this is only a problem for things where your simulation is say a NR result, but your search is a phenom model?

added 2 commits

• a8f0f3f4 - update conversion to not remove search keys
• 48dcbbc2 - Merge branch 'tify-up-of-inject-signal' of git.ligo.org:Monash/tupak into tify-up-of-inject-signal

Compare with previous version

I just pushed a fix to get around the first issue I had. The wg now no longer deletes search parameters, only added parameters.

I'm good for this to merge if the tests succeed.

mentioned in commit 86331d59

merged

mentioned in merge request !85 (merged)