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

Issue 294 fourier model freq not found #299

Closed dbenn closed 1 year ago

dbenn commented 1 year ago

Hi @mpyat2

It took awhile to arrive at a solution I was happy with for this. The Fourier model uncertainty component that went from top hit to full result data for FWHM has been modified from a frequency "double tolerance is-close" approach to use a "period analysis data point object hashcode" approach.

It seems to work well. If you have the time, could you please review and try the change?

Thanks.

David

ps - there's a lot of very small changes; sorry @mpyat2; I've had a few reports of this bug now

mpyat2 commented 1 year ago

I took KP Lyn star, all times, Johnson V, Heliocentric converted.

DC DFT Low Period = 0.0759, High period = 0.0760, resolution 0.0000001

For the top period 0.0759334 d I created a model (with a button Create Model of the Period Analysis (DC DFT) window).

Then I inspected the model properties.

Model Information [Uncertainty]


For frequency 13.169435320952, period 0.0759334, power 1335.077392316813, semi-amplitude 0.201663218973:

FWHM for frequency: Lower bound: 13.169504694928 Upper bound: 13.16934860451 Resulting error: 0.000078045209

Standard Error of the Frequency: 0.00000126846 Standard Error of the Semi-Amplitude: 0.00223403423


Then I tried another period 0.07593 d (close to the top period). It is not on the list, I just enter the value in the "Period (days)" dialog after pressing the Create Model button.

Model Information [Uncertainty]


For frequency 13.170025023048, period 0.07593, power 1335.077392316813, semi-amplitude 0.201663218973:

FWHM for frequency: Lower bound: 13.169504694928 Upper bound: 13.16934860451 Resulting error: 0.000078045209

Standard Error of the Frequency: 0.000002205736 Standard Error of the Semi-Amplitude: 0.003884781801


For me, the result is somewhat strange. Actually, the Model Information dialog shows information for the top frequency 13.169435320952 (power, semi-amplitude, FWHM). Standard Error differs, though.

It looks something weird to me...

dbenn commented 1 year ago

Thanks @mpyat2. Good pick-up!

Before the bug fix changes, there was a check for whether the submitted frequency (zeroth harmonic) was actually a top-hit for FWHM. I need to re-instate that. Then, since for FWHM the zeroth selected harmonic must be a top hit, one of a few things can happen if it's not:

  1. silently omit FWHM from the uncertainty result;
  2. open a warning dialog to say that FWHM will not be generated because the specified frequency can not be matched with a top hit;
  3. open an error dialog to say that no uncertainty result will be generated because the specified frequency can not be matched with a top hit.

We can still generate std error values since they are based upon the user submitted frequencies and model residuals, so I'm inclined to go for option 2. What do you think?

The only thing is that I will have to again use a tolerance based is-close approach to ask whether the submitted frequency is the zeroth top hit. Either that or simple equality since they should be identical. Or, a string based approach. I may also have to make sure that what is in the dialog (text) has the same number of decimal places as what is in the top hit result first. The string based approach could help with that.

Thoughts welcome as usual.

dbenn commented 1 year ago

Hi @mpyat2. I've added a check for top-hit back and open a warning dialog if a top hit has not been specified.

I was tempted to use simple numeric equality but a little wary of that, so convert to strings with many digits.

Let me know what you think.

mpyat2 commented 1 year ago

Hi @dbenn, I think the string-related approach would be ok.