AAVSO / VStar

VStar is a visualisation and analysis tool for variable star data brought to you by AAVSO
https://www.aavso.org/vstar
GNU Affero General Public License v3.0
10 stars 3 forks source link

Creating Fourier models sometimes fails with "cannot find specified frequency" error #294

Closed dbenn closed 1 year ago

dbenn commented 2 years ago

Creating Fourier models sometimes fails with "cannot find specified frequency" error:

cannot find freq
dbenn commented 2 years ago

This is caused by the delta between actual frequency value and searched frequency value when determining period error. See PeriodAnalysisDerivedMultiPeriodicModel.{findIndexOfFrequency(),fwhm()}.

dbenn commented 2 years ago

One approach is just to reduce the delta (tolerance) in the calls to Tolerance.areClose(), e.g. from 1e-9 to 1e-6, confirmed by quick code changes and re-runs.

I'm not sure whether this will suffice however, since some frequency values may have even less decimal places (e.g. 3), as reported here by Aritra in the most recent CHOICE course: https://www.aavso.org/week-four-section-iii-cleanest

Perhaps using the numeric preferences values for periods/frequencies ("other") to dynamically determine the necessary tolerance could work.

Needs some thought.

dbenn commented 2 years ago

We could also allow period error to be disabled by the user.

dbenn commented 2 years ago

Another approach would be to take a different approach to looking up the necessary data in period search results that leads to this bug (e.g. result line hashcodes).

dbenn commented 2 years ago

Matthias Kolb emailed me with another example of this error.

dbenn commented 2 years ago

I'm leaning towards the tolerance based approach, e.g. 1e-5 vs 1e-9. Also, changing the scope of the exception catch block in the model class's PeriodAnalysisDerivedMultiPeriodicModel.toUncertaintyString() (line 171), since this is where the failure is currently being exposed.

dbenn commented 2 years ago

The problem is however, that it may not matter how much the tolerance is tweaked: it's no guarantee of always finding the right value. Better would be storing the indices in the top hits and full result tables in the model or algorithm object. Or, the hashcode of values could be used. Shouldn't the hashcode of the same interned double value, or row of doubles, in each table be the same? Test that.

dbenn commented 2 years ago

It gets weirder. The output of 3 matched full result and top hits table row pairs:

5.667367, 5.667360
5.667367, 5.667360
5.667367, 5.667360
5.671000, 5.670992
5.671000, 5.670992
5.671000, 5.670992
5.674114, 5.674113
5.674114, 5.674113
5.674114, 5.674113 
dbenn commented 2 years ago

The internal handling will be simplified if we know that a frequency/period selection for model creation can only ever be a top-hit. The simplest way to do this is to remove the model button from the data table pane, leaving it only on the top hits data pane. There's no reason to create a model from the full data pane and since it's not possible to do that from the DCDFT result plot panes either, removing the model button from the data pane makes sense. The user manual doesn't make reference to it either.

dbenn commented 2 years ago

Using the hashcode of a PeriodAnalysisDataPoint to match the top hit against result data works well.

Actually we, only need the top-hit full result index in fwhm() so will call it from there. Other values like semi-amplitude and power can be obtained from the top-hit data point passed into the model class constructor.

dbenn commented 1 year ago

Matthias Kolb found a case in which the "no FWHM generated" dialog opened after a DCDFT model creation yet the FWHM was added to the models dialog:

For frequency 5.663989, period 0.176554, power 104.433213, semi-amplitude 0.084729:

  FWHM for frequency:
        Lower bound: 5.664
        Upper bound: 5.665
     Resulting error: 0.0005

  Standard Error of the Frequency: 0.000079
  Standard Error of the Semi-Amplitude: 0.003941

Upon inspection, this seems to be because I've kept a redundant check in place in fwhm() and also added the dialog invocation in the wrong place, while thereafter, successfully continuing with the FWHM calculation anyway!

dbenn commented 1 year ago

The good news is that anyone with the current snapshot (before this fix) will get the expected result (with a FWHM), just with the annoying bogus dialog. Will fix this though of course.

dbenn commented 1 year ago

Because of the extent to which the top hit is tied to all uncertainty values, for a non top hit freq/period value, no uncertainty will be generated at all now. An unambiguous way to determine whether a top hit frequency was specified has been implemented now. Fairly nuanced messages are written to the Models dialog's uncertainty pane now.