Fixed failing PANSTARRS Catalog queries
#2727
Conversation
|
@jaymedina your comment says there are screenshots, but I'm not seeing them; were they unintentionally omitted? |
No I just hadnt added them yet, I've updated the description now. |
jaymedina
force-pushed
the
panstarrs-queries
branch
from
5780b99
to
f9cb352
Compare
|
This is ready for a pass through. There are 2 CI tests failing due to something upstream. I triggered the CI tests in my other open PR (#2693) and it failed with the same errors, so I'm going to make a separate PR to resolve that issue and rebase this branch (and my other branch) onto main once the fix has been merged. This code is ready to go, though. Let me know if you have any questions/comments! Thanks |
|
|
||
| # Parameter syntax needs to be updated only for PANSTARRS catalog queries | ||
| if service.lower() == 'panstarrs': | ||
| catalogs_request.extend(self._build_catalogs_params(params)) |
There was a problem hiding this comment.
This call updates the syntax of the parameters so it's readable in the backend. It needs to be made here so that users can add criteria decorators to their PANSTARRS queries, despite it being an API that requires params to be passed as JSON. It returns the params in list format, so I then have to convert it back to dictionary so that it can be dumped as a JSON dict later in the request.
| catalogs_request[key] = catalogs_request[key][0] | ||
|
|
||
| # Otherwise, catalogs_request can remain as the original params dict | ||
| else: |
There was a problem hiding this comment.
This else statement will ignore all the logic from above if the service is not the PANSTARRS API. This is because the MastMissions API also requires params to be passed as JSON, but doesn't jive with the parameter syntax transformation that happens above. Namely because of the way the coordinates are passed for MastMissions.
| @@ -691,6 +691,17 @@ def test_catalogs_query_criteria(self): | |||
| assert isinstance(result, Table) | |||
| assert 'PSO J254.2861-04.1091' in result['objName'] | |||
|
|
|||
| result = mast.Catalogs.query_criteria(coordinates="158.47924 -7.30962", | |||
There was a problem hiding this comment.
For the unit test, I'm using one of the queries that was reported to fail by one of our users. It includes the columns and sort_by arguments, both arguments were causing the 422 error observed by both users. This unit test will catch any future issues with the criteria syntax, should they arise.
|
I think we'll wait to merge this til the dev dependency issue is resolved? Unless that one looks like it's going to take a long time. |
I also think it's best to wait until I open up a PR addressing those tests and get that merged in. I'll get started on that later today. Thanks for the review! |
|
@jaymedina #2730 and #2729 fixed CI - rebase? |
jaymedina
force-pushed
the
panstarrs-queries
branch
from
8bb494a
to
c35da32
Compare
jaymedina
force-pushed
the
panstarrs-queries
branch
from
2033476
to
c42dd7a
Compare
|
This pull request claims to address both #2709 and #2716, but because it didn't properly follow GitHub's syntax for linking multiple issues to a pull request then only #2709 was closed when it was merged and #2716 remains open. |
|
Thanks for the heads up. |
Summary
Resolves #2709
Resolves #2716
Users were reporting issues where PANSTARRS queries were failing when columns were specified, or any column sorting was requested with a sorting direction.
The solution I found was:
collections.py)_build_catalogs_paramswhenuse_jsonis true for PANSTARRS queries (see changes inservices.py). This call updates the syntax of the parameters being passed so that users can use criteria decorators (e.g.sort_by=[("asc", "distance")]), but settinguse_jsonto true would prevent these decorators from passing because the synax wasnt being updated (params was left as the originalparamsdictionary so it can be converted to a JSON object in the request). This didn't seem to be a problem before, but now that the PANSTARRS API needs the parameters to be passed as JSON dictionaries, this call needed to be made for bothuse_json= False AND True. Notice how none of theMastMissionsexamples have criteria decorators. This is becauseuse_jsonis True so the params are passed in a dictionary without updating the syntax. That's something we can improve upon MastMissions the way I've done here with PANSTARRS, but is out of scope of this PR, so I'll make a separate issue about it.Task list
CHANGELOGentryScreenshots
When sorting direction is specified,
(direction, criteria)tuples can be passed as a single tuple or list of tuples:The changes in this PR do not break non-PANSTARRS queries to the Catalogs API:
The changes in this PR do not break non-PANSTARRS queries that request specific columns and/or sorting:
Below are screenshots (taken May 4, 2023) showing the results of the queries that reported errors, after fix:
#2709:
#2716: