Skip to content

Conversation

kpuriIpac
Copy link
Contributor

@kpuriIpac kpuriIpac commented Aug 26, 2025

Ticket: FIREFLY-1787

  • Note: This PR addresses the first part of the ticket, and not the second part. I wasn't sure about the second part, but we can discuss it over zoom, and I can then include it in this or another small PR.
    • First part pre-filters the position columns for Spatial Search by UCD as specified in the ticket.
  • Question: Do we need to pre-filter columns elsewhere? This PR only does it for Spatial Search on TAP.

Testing: https://fireflydev.ipac.caltech.edu/firefly-1787-tap-col-improvement/firefly

  • Open the column picker for TAP's Spatial Search position columns. They should have some pre-filtered UCDs according to the ticket.

@kpuriIpac kpuriIpac added this to the 2025.4 milestone Aug 26, 2025
@kpuriIpac kpuriIpac requested review from robyww and gpdf August 26, 2025 01:04
@kpuriIpac kpuriIpac self-assigned this Aug 26, 2025
Comment on lines 379 to 380
{fieldKey: lonKey, doQuoteNonAlphanumeric, colTblId: 'posCol', onSearchBtnClicked: () => filterMappedColFldTbl(cols, 'posCol', 'pos.eq.ra,pos.galactic.lon,pos.ecliptic.lon')},
{fieldKey: latKey, doQuoteNonAlphanumeric, colTblId: 'posCol', onSearchBtnClicked: () => filterMappedColFldTbl(cols, 'posCol', 'pos.eq.dec,pos.galactic.lat,pos.ecliptic.lat')}
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to support pos.eq. This is when the ra and dec are in the same column.
test on TAP with CANFAR -> pgm -> gaiadr2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. If I just add pos.eq though, we'll end up seeing all ra/dec possibilities for both lon and lat col.

I did notice that for the table you mentioned, the single value for both ra/dec (named pos) has the following UCD:
pos.eq;meta.main

And normally for other ra/dec values, meta.main doesn't show up. Can I assume meta.main always shows up for these same column ra/dec values?

In that case my filter could become something like :

pos.eq;meta.main, pos.eq.ra, pos.galactic.lon, pos.ecliptic.lon

Let me know what you think.

Copy link
Contributor

@robyww robyww Aug 27, 2025

Choose a reason for hiding this comment

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

It might take a little more code but pos.eq does not have to match pos.eq.ra. If you can’t make that work then pos.eq;meta.main is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about this a little more. Yes, just go with pos.eq;meta.main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, yes with a little more code I can get your first suggestion going, but if pos.eq;meta.main works just as well then yes that's much more straightforward.

@kpuriIpac kpuriIpac force-pushed the FIREFLY-1787-tap-col-improvement branch 2 times, most recently from ca25459 to f7bb1e6 Compare August 27, 2025 23:42
@kpuriIpac
Copy link
Contributor Author

@robyww so I was stuck on this for a bit running into some bugs, but I think we only support one OR in our filters currently. But it's not obvious that we don't support more, which is why it took me some time to track it down too.

So if I have a filter like:

pos.eq.ra OR pos.galactic.lon OR pos.eq;meta.main (any number over 2)

then only the first filter gets applied. But if we have only 2 (or 1) filters, then they both get applied.

I'm happy to work on a fix for this later (unless this is intended behavior for some reason) but I wasn't able to get to it in time for tomorrow. But according to the ticket, @gpdf mentioned to use equatorial values if we can't do ORs. But since we can do one OR at least, I did:

pos.eq.ra OR pos.eq;meta.main and pos.eq.dec OR pos.eq;meta.main

So this will at least support the 2 most common use cases for this filter (you can try this on the table with single column for both ra/dec you mentioned above)

Copy link
Contributor

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

@kpuriIpac kpuriIpac force-pushed the FIREFLY-1787-tap-col-improvement branch from f2b4342 to de1da84 Compare August 28, 2025 18:28
@kpuriIpac kpuriIpac merged commit 4ac2ac5 into dev Aug 28, 2025
@kpuriIpac kpuriIpac deleted the FIREFLY-1787-tap-col-improvement branch August 28, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants