search

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

Filter on "quality" for QLP fixed #322

Closed mpyat2 closed 1 year ago

mpyat2 commented 1 year ago

Fixed #321: "ob.addDetail("QUALITY", ...)": the first parameter must be in uppercase.

orionlee commented 1 year ago

The PR fixed the issue for QLP. I'm wondering:

  1. Is there any use case for non upper case key in ob.addDetail(key, ...) then?
  2. A loosely related issue: it'd be great if there is some way to know the names for the columns to be used in a VeLA filter. Right now one can find out some of them from the examples in documentation. It might be a headache to document them given they are source specific. Maybe a list of valid names for the current source can be shown in somewhere in VStar UI?
mpyat2 commented 1 year ago

I've found non-capitalized parameters in CatalinaSkySurveyObservationSource

            observation.addDetail("MasterID", fields[0], "Master ID");
            observation.addDetail("RA", fields[3], "RA");
            observation.addDetail("Dec", fields[4], "Dec");
            observation.addDetail("Blend", fields[6], "Blend");

and in ASASSNObservationSource:

                observation.addDetail("Camera", fields[2], "Camera");

As for ASASSNObservationSource, I've just fixed it as part of #323

mpyat2 commented 1 year ago

Concerning VeLa filter on "observation details" created with the "observation.addDetail()" function: currently, the "details" added with this function are always strings, even if they look like numbers. This means that VeLa comparison operators also use "string" logic, for example, flux > 100 (for the ASAS-SN plug-in) will be true for the value 90. It is somewhat frustrating yet it is the current implementation. Am I right, @dbenn ?

orionlee commented 1 year ago

If we do want the key to be in uppercase, would it be more defensive if ValidObservation always normalizes the key to be in uppercase (and document it as such)?

(Even if ValidObservation is made to normalize the keys in uppercase, this PR is good anyway, helping to reinforce the uppercase requirement.)

dbenn commented 1 year ago

Concerning VeLa filter on "observation details" created with the "observation.addDetail()" function: currently, the "details" added with this function are always strings, even if they look like numbers. This means that VeLa comparison operators also use "string" logic, for example, flux > 100 (for the ASAS-SN plug-in) will be true for the value 90. It is somewhat frustrating yet it is the current implementation. Am I right, @dbenn ?

That is the case currently @mpyat2 but I agree that typed observation details would be a Good Thing, and is again one of these things I've thought about.

Please create an issue specifically for this.

dbenn commented 1 year ago

@mpyat2, @orionlee, thanks for your comments. I'm mulling over some things re: this. VeLa itself ignores variable case. So, for example, you can have a filter like these which are all equivalent:

quality = 512

and

Quality = 512

So yes, case should be ignored in general.

dbenn commented 1 year ago

The PR fixed the issue for QLP. I'm wondering:

  1. Is there any use case for non upper case key in ob.addDetail(key, ...) then?
  2. A loosely related issue: it'd be great if there is some way to know the names for the columns to be used in a VeLA filter. Right now one can find out some of them from the examples in documentation. It might be a headache to document them given they are source specific. Maybe a list of valid names for the current source can be shown in somewhere in VStar UI?

Please create an issue for 2. Thinking about 1...