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

Simple filter fails with more than one Phase condition #319

Closed mpyat2 closed 1 year ago

mpyat2 commented 1 year ago

Simple filter Phase > -0.1 works, Phase < 0.1 also works, however, when I try both conditions, a warning occurs (No observations). image image image image

dbenn commented 1 year ago

Yes, thanks @mpyat2. I think I know what's going on here. I'll reply with details tomorrow (after midnight and I need to get up early).

dbenn commented 1 year ago

Oh, so that's weird. The equivalent VeLa filter works and the simple filter description reported in the Previous Filters dialog for that simple filter is identical to that VeLa filter!

dbenn commented 1 year ago

Okay, so what's happening is that PhaseFieldMatcher is actively changing values less than zero into standard phase values by adding one. This also requires the relational operators to be changed (e.g. > to <=) if a < or > is involved but the code was not doing this.

There was no unit test for PhaseFieldMatcher, so not surprising this was missed.

So, either the relational operator needs to be changed or only positive values are allowed. If the latter, to express a filter such as phase < 0.1 or phase > 0.9 would not be possible with the simple filter dialog since disjunction is not supported. A VeLa filter would have to be used.

Another approach would be to allow standardphase and previouscyclephase explicitly both here and in for VeLa filters (see #320). I'm starting to be inclined to do this. If this is done, the user manual change in #320 will need to be reverted. For VeLa, phase could possibly be retained as a synonym of standardphase.

mpyat2 commented 1 year ago

Hi @dbenn , explicit standardphase and previouscyclephase is a good idea

dbenn commented 1 year ago

This works now: previouscyclephase > -0.1 or standardphase < 0.1

dbenn commented 1 year ago

I'm considering #319 and #320 together here, hence the VeLa example above. I think that any simple filter and VeLa filter changes made re: phase can just happen on #320's branch.

dbenn commented 1 year ago

One thing to keep in mind @mpyat2 is that since previouscyclephase = 1 - standardphase, a filter that selects standardphase = N (or phase = N) will also correspond to 1 - N (previouscyclephase). These in fact correspond to the same observations.

A simple way to see this is with this VeLa filter:

standardphase > 0.69 and standardphase < 0.71

or the equivalent:

phase > 0.69 and phase < 0.71

Note also that to get discrete subsets of phase, disjunction will have to be used, e.g. try this:

(standardphase > 0.69 and standardphase < 0.71) or
(previouscyclephase > -0.81 and previouscyclephase < -0.79)

This is not possible with a Simple (dialog based) Filter, only with a VeLa (or custom) filter, since Simple Filters are implicitly conjunctive, e.g. the equivalent of phase > 0.2 and phase < 0.8 is possible but not phase > 0.2 or phase < 0.8.

So, I would be inclined to simplify Simple Filters, specifying only Phase (standard phase) and clarify all this in the user manual, changing what I've already added there (in #320).