Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter on "quality" for QLP fixed #322

Merged
merged 1 commit into from
Jan 14, 2023

Conversation

mpyat2
Copy link
Collaborator

@mpyat2 mpyat2 commented Jan 10, 2023

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

@mpyat2 mpyat2 added bug Something isn't working plug-in Plug-in Development labels Jan 10, 2023
@mpyat2 mpyat2 requested review from orionlee and dbenn January 10, 2023 17:45
@mpyat2 mpyat2 self-assigned this Jan 10, 2023
Copy link
Collaborator

@dbenn dbenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @mpyat2! I'll give @orionlee a chance to review it as well before merging.

@orionlee
Copy link
Collaborator

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
Copy link
Collaborator Author

mpyat2 commented Jan 11, 2023

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
Copy link
Collaborator Author

mpyat2 commented Jan 11, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

dbenn commented Jan 12, 2023

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
Copy link
Collaborator

dbenn commented Jan 12, 2023

@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
Copy link
Collaborator

dbenn commented Jan 12, 2023

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...

@dbenn dbenn merged commit 1ae8c06 into master Jan 14, 2023
@dbenn dbenn deleted the issue-321-filter-on-quality-fails-for-QLP branch January 14, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working plug-in Plug-in Development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter expression like "quality = 512" works for TESS files, but not for QLP
3 participants