SciPy filters

[ravioli1369]

Adds scipy filters

[EthanMarx]

This is great, thanks Ravi. Could you add a few tests agains the scipy implementation? I know you said edges are slightly off, but we can just test the middle of the waveform.

[wbenoit26]

Maybe we start a filters module and put this and spectral.py in there? Not sure if we'll have other functions though

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
ml4gw
__init__.py
ml4gw/dataloading
hdf5_dataset.py
ml4gw/transforms
iirfilter.py
Project Total

This report was generated by python-coverage-comment-action

[EthanMarx]

Yeah Ravi and I were also not exactly sure what to do about this. I do like the idea of a filters module. I'll suggest we hold off for now until we have a better idea where this should live.

@ravioli1369 one addition that would be great is to add a Highpass torch.nn.Modleclass to the transforms module. in the __init__, you would build all of the relevant filter coefficients dependent on user defined parameters. Then in the forward call you would perform the actual filtering with

[wbenoit26]

I'll agree with holding off. I think "rule of three" - if there's a third function similar to these, we can group them all together. It wouldn't surprise me to end up with a signals module like gwpy has.

[deepchatterjeeligo]

This will default to the CPU. We may have to take the approach of using nn.Module instances like we do for the waveforms.

[EthanMarx]

Yeah thats right. I think we can keep the functions (any created tensors should by default be made on data.device) and then also have a torch.nn.Module wrapper like we do for the other transforms that builds the necessary coefficients and saves them as buffers so they get sent to device

[ravioli1369]

@deepchatterjeeligo @EthanMarx could you guys take a look at the current implementation to see if it solves this?

[EthanMarx]

@ravioli1369 I would keep a filters.py module with the functions at the top level where it was.

Then, also add a highpass.py or butterworth.py module to ml4gw/transforms that implements the torch.nn.Module wrapper. See how we do this for the Whitener and SpectralDensity

[EthanMarx]

Thanks for this Ravi looks good. I think this will work fine. In most cases users will be building filter coefficients on CPU to later be used on GPU which is exactly what the IIRFilter module allows us to do via calling module.to(device).

Also thanks for fixing those relative imports. Won't make you do this here but down the line I think we as a group should be a bit better about splitting out PRs into individual tasks for better repo management.

[ravioli1369]

@EthanMarx yea I realized there were too many changes with the current PR and was working on putting the relative imports in another PR anyways.

[wbenoit26]

Looks great, Ravi, I'm just requesting two minor changes

[ravioli1369]

@wbenoit26 @EthanMarx looks like the phenom_p tests for the far (>400 Mpc) waveforms are failing, could we get this in before I check the tolerance for the phenom_p one?

[EthanMarx]

@ravioli1369 Would rather wait on this PR til I test it out with my PR comparing against lalsimulation waveform conditioning.
Going to clean that up today and get this incorporated.

[wbenoit26]

After this and the TimeDomainCBCWaveformGenerator get merged, I think it would be good to make a 0.7 release

[EthanMarx]

@ravioli1369 no point in repeating verbatim the scipy documentation.

I would just point users to their documentation.

[ravioli1369]

I think it would be still good to add the documentation so that it at least appears on hover? Anyways it's mentioned that it's a wrapper for scipy

[EthanMarx]

@ravioli1369 Was able to test this out and get good agreement with the filtering done by lalsimulation. So, if you can rebase onto main and address the comments I left we can get this in

[EthanMarx]

@ravioli1369 any idea why these tests are failing?

[wbenoit26]

One of the failures is from spectral.py. It would be good to benchmark the failure rate across all of our probabilistic tests to set better tolerances.

[wbenoit26]

I'm happy with how all of this looks now. @EthanMarx, are you good as well?

[EthanMarx]

Yes looks good. Thanks a ton Ravi