search

nuclear-multimessenger-astronomy / nmma

A pythonic library for probing nuclear physics and cosmology with multimessenger analysis
https://nuclear-multimessenger-astronomy.github.io/nmma/
GNU General Public License v3.0
30 stars 58 forks source link

trigger-time flag setting #256

Closed tylerbarna closed 11 months ago

tylerbarna commented 11 months ago

Describe the bug Currently, trigger-time is an optional flag for light_curve_analysis, but it's effectively a mandatory parameter in order for analysis to run. For example, for a lightcurve with an initial time of t=4000, not setting the trigger time will cause the log likelihood to always be negative infinity, which I suspect is due to the trigger time being set to something like t=0 if not specified

To Reproduce Steps to reproduce the behavior:

  1. generate an injection and lightcurve for any model
  2. run light-curve-analysis without specifying trigger-time
  3. see as your terminal is filled with low likelihood warnings

Expected behavior lightcurve analysis should proceed regardless of whether or not the trigger time is specified. Trigger time should default to the first data point time, with the option of specifying a different time

Platform information:

Additional context This should be fixed by

Alternatively, we could modify the trigger-time behaviour in the backend to be relative to the start time of the lightcurve, which is how the tmin and tmax parameters work, but that would be a discussion to have with @mcoughlin, as I'm not sure how much modification that would require

sahiljhawar commented 11 months ago

Instead of raising warning, why not make it a required parameter?

tylerbarna commented 11 months ago

Instead of raising warning, why not make it a required parameter?

I think the default use case is to set the trigger time as the first data point, so I believe it would make more sense to have the trigger time set automatically to be that with the optional choice to set a different trigger time

sahiljhawar commented 11 months ago

If I undestand it correctly, trigger time is set to the first data point if the data file is available else it defaults to the sampling tmin. In either cases a warning should be raised that trigger time not given and a default value will be used (based on previous)

tylerbarna commented 11 months ago

@sahiljhawar I believe the current behaviour is that the trigger time is set to 0 if the trigger time parameter isn't set, my previous comment was moreso regarding what the ideal implementation would be