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
9 stars 3 forks source link

339 user created series do not persist across additive loads #348

Closed dbenn closed 1 year ago

dbenn commented 1 year ago

@mpyat2 There's a number of things going on in this pull request and the changes are deep in the guts of VStar, so feel free to ask questions. It took me awhile to work through and test this. A review from you is the key thing. Thanks.

mpyat2 commented 1 year ago

I noticed a strange behavior:

  1. New Star from AAVSO... a) RR LYR, from 2459103 to 2459104, Johnson V b) View->Filter from plot c) View->Create series...->Filtered (select some distinct color, i.e. red) d) Tools->Heliocentric JD converter (only the original series will be converted, this is an old 'feature' (or bug?))
  2. New Star from AAVSO... a) RR LYR, from 2459096 to 2459097, Johnson V, Add to current = yes

Now zoom the first part, say, from 2459103.35 to 2459103.39. You will see doubled V observations: both JD and HJD are visible.

image

This strange duplication existed before and is not directly connected with the current changes, but I only noticed it now. The duplication appears only if a "New series" is created.

dbenn commented 1 year ago

I can reproduce this on the current branch but not with 2.22.0.

Step 1. b) is not needed BTW. You can just create a series via View -> Create Series using Johnson V (vs Filtered).

The duplicate observations must be entering through the new-series-in-presence-of-additive-load logic.

dbenn commented 1 year ago

Okay, fixed! Geez! This one drove me crazy!

See last commit, but in the end, all the craziness I added/changed in Mediator was not necessary. Which is good, because it felt wrong, had a bad smell. Seemed like an anti-pattern.

Instead, a 2 lines change in AbstractObservationRetriever.categoriseValidObservation() was all that was required to properly handle user-defined series. Please check again @mpyat2.

Note that this last commit does not address the user-defined-series-does-not-get-HJD-converted bug you noted above. We can either do that here or in another issue.

dbenn commented 1 year ago

Hi @mpyat2, I've fixed the HJD conversion bug too.