Skip to content

MFDv5: enforce --SFTWindowType when using --noiseSFTs

David Keitel requested to merge david-keitel/lalsuite:mfdv5-enforce-window-for-noisesfts into master

Description

Thanks to work by @rodrigo.tenorio on PyFstat we noticed that Makefakedata_v5 claims in the help string the following:

--SFTWindowType=<string> [default: NULL]
           Window function to be applied to the SFTs (required when using --noiseSFTs)

But there has not been any such requirement actually enforced in the code. The older code MFDv4 on the other hand has had that check since 2012 and still does on current master, hence this appears to be a simple oversight from when MFDv5 was coded up.

API Changes and Justification

Backwards Compatible Changes

  • This change introduces no API changes
  • This change adds new API calls

Backwards Incompatible Changes

  • This change modifies an existing API
  • This change removes an existing API

If any of the Backwards Incompatible check boxes are ticked please provide a justification why this change is necessary and why it needs to be done in a backwards incompatible way.

Review Status

This may break some people's CW injection workflows, but to quote @evan.goetz from https://git.ligo.org/CW/software/lalsuite/-/issues/84 it is actually an essential sanity check:

The reason to enforce the user to specify a window--"rectangular" or "Hann", for example--is so that the user has done their due diligence to understand the noise SFTs that are being provided. Standard SFTs created from real data are often Tukey windowed, so the user should actually NOT specify rectangular as this will lead to a slightly incorrect result.

If people really want to still skip windowing, they can always do --SFTWindowType=rectangular as I've done to make the test script pass -- note from here that MFDv4 treated "None" as "rectangular", while MFDv5 calls XLALCreateNamedREAL8Window and that only accepts the explicit window name.

For approval @karl-wette , cc @keith-riles in case you have specific objections of some important workflow breaking.

PS: I know I could have made this a XLAL_CHECK one-liner, but it seems more readable and fits better with the surrounding code blocks this way.

Edited by LALSuite Bot

Merge request reports