Skip to content

lalapps_PredictFstat: simplify input logic, cleanup plus modernize some internals

Reinhard Prix requested to merge CW/software/lalsuite:RP-PredictFstat-simplify-input-logic-and-cleanup-plus-modernize into master

After Max's nice improvements to PredictFstat (https://git.ligo.org/CW/lalsuite/merge_requests/18), I've noticed that this code could still use some more modernization of internals, and simplification of the rather tricky new input logic. [I'm actually using this code quite a bit, it's very useful, and so this time+effort investment to keep it fresh and reduce code-entropy seems worthwhile to me].

Change summary:

  • removed the --SFToverlap option, which doesn't work here and would yield wrong results
  • found+fixed a major bug (entirely my fault), namely --transientTauDays was actually interpreted as seconds, @david-keitel you might want to check if you've ever been hit by this.
  • Improved help string, using sections and better structuring to hopefully make the various options clear to the user
  • use RAJ,DECJ,EPOCH user input types where appropriate
  • new API functions XLALRead[Multi]TimestampsFileConstrained() to read timestamps file and return only timestamps within given time-window [minGPS,maxGPS)
  • use --minStartTime and --maxStartTime to constrain either 1) loaded SFTs, 2) loaded timestamps, or 3) construct gapless timestamps depending on user input, removed the now-redunant options --startTime+duration [this had only been introduced in the previous merge, so no need to deprecate/defunct them, I've just removed them].
  • don't allow passing --IFOs together with --DataFiles, which would actually not do anything and be only confusing
  • defuncted the obsolete --SignalOnly switch

David, I noticed the transient-related input arguments differ from CFSv2 now, do you think it would be worthwhile to use the same syntax here as in CFSv2 to make this less confusing when going from one to the other? Are this options currently used anywhere or could we just directly defunct & replace them to avoid complicating the input logic?

@maximillian.bensch & @david-keitel could you please look over this and see if there's any red flags? I'm assigning this to Karl for actual merging, but please comment if you have any concerns with these changes.

Merge request reports