Skip to content

fix LALCleanCOMPLEX8SFT for lines wider than width

David Keitel requested to merge david-keitel/lalsuite:SFTclean-wide-lines into master

Description

Fixes #431 (closed): Lines with wings wider than width (called --maxBins in lalapps_SFTclean) currently simply do not get cleaned at all. Expected behaviour: it should clean within +-maxBins at least. The roblem is exposed with a simple addition to the test script in the first commit, resulting in zero cleaning where actually all bins of the test SFTs should be changed:

Files dump_L-1_L1_1800SFT_mfdv4-*-1800.sft.txt and dump_L-1_L1_1800SFT_cleaned-*-1800.sft.txt are identical
error: dump_L-1_L1_1800SFT_mfdv4-*-1800.sft.txt and dump_L-1_L1_1800SFT_cleaned-*-1800.sft.txt should differ by exactly NSFTs*19=3*19=57 line(s).

What the ages-old docs have to say for LALCleanCOMPLEX8SFT:

There is also a width parameter. This is an upper limit on the number of bins that will be cleaned on either wing. Thus, if width is specified to say 3, then only a maximum 3 bins will be cleaned on either side irrespective of the actual wing size specified. This parameter is present for historical reasons, and should probably be removed. Currently, it is recommended to set this sufficiently large (larger that any wing size in bins) so that it has no effect.

As indicated, a workaround with the previous code version is to just set --maxBins high enough, but note that for narrow-band SFTs, it still has to be larger than the line width: even if setting it equal to the number of bins actually in the SFTs, still no cleaning may happen.

Actually removing that "historical" parameter wouldn't look too difficult at the level of this function, but would require a depreciation cycle for all its callers. Instead, I think my second commit here actually brings the function's behaviour in line with what its documentation claims and what a user would reasonably expect.

One still will have to be super-careful in manually setting the value higher if required, and checking the output SFTs because there's no verbose output from the code. Marking this as Draft for now while I think about how a sane default or clear warning can be given at the lalapps executable level.

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

Review Status

@luana.modafferi found the initial problem. @karl-wette could you in the first place please confirm my reading of the documentation regarding the intended behaviour (I think the specific example Thus, if width is specified to say 3, then only a maximum 3 bins will be cleaned on either side irrespective of the actual wing size specified is exactly what I'm changing it too, and not what it did before, which was then no bins will be cleaned on either side if the actual wing size specified is larger than 3), and then the fix's logic.

Edited by David Keitel

Merge request reports