Skip to content

lalapps_ComputePSD overhaul: modularize, fix some minor bugs, add one new feature

David Keitel requested to merge david-keitel/lalsuite:modularize-ComputePSD into master

Description

As previously discussed in https://git.ligo.org/CW/software/lalsuite/-/issues/73 and https://git.ligo.org/CW/software/lalsuite/-/merge_requests/78 I started modularizing the CW code lalapps_ComputePSD by moving most of its internals into lalpulsar XLAL functions, so that they can be maintained better and also called through SWIG. Along the way, I extended the test script for this code and fixed two small bugs:

  • An ancient off-by-one error in the homebrew median implementation for even-length arrays. See https://git.ligo.org/CW/software/lalsuite/-/issues/79. (This must have affected results "in the wild", but to a negligible level, since CW applications usually use a large number of SFTs.)
  • At one point, arrays were silently sorted in place, which could not cause any problems as long as it was a monolithic executable, but would have become a bug now as a callable library function. See https://git.ligo.org/CW/software/lalsuite/-/issues/78.

New feature:

  • The different "math ops" (e.g. means, medians) can now be called both by their integer numbers and by human-friendly string names.
  • Introduced functions XLALRoundFrequencyDownToSFTBin() and XLALRoundFrequencyUpToSFTBin() in the base SFTfileIO module to make sure there are consistent conventions for these.
  • New option --normalizeByTotalNumSFTs for emulating harmonic/powerminus2 means over combined set of SFTs from all IFOs.

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

I think this should actually be fully backwards compatible; it only "modifies" the commandline options of lalapps_ComputePSD insofar as it adds a new switch and makes the parsing of the existing --*mthop* options more flexible, but should not break any old commandlines. @karl-wette please verify.

Review Status

@rodrigo.tenorio has run some real-world tests of the new --normalizeByTotalNumSFTs feature. The extended test script covers much more of the basic functionality as before. @karl-wette gave initial comments, but please have a careful look at this mature version again.

I'd prefer this to be merged without squashing; I've cleaned up the source branch somewhat compared to https://git.ligo.org/CW/software/lalsuite/-/merge_requests/78 and the remaining commits are all either independent changes, or where they are more incremental, will make it much easier to track any changes back to the old version.

Merge request reports