Allspec: code for /allspec_apred_vers_apstar_id_file_spec - #97
Allspec: code for /allspec_apred_vers_apstar_id_file_spec#97astronomygupta wants to merge 5 commits into
Conversation
havok2063
left a comment
havok2063
left a comment
There was a problem hiding this comment.
This is a good start, but I think we want to make this a bit easier and more useful for users, and do some performance/timing benchmarks. I've made some comments and requested some changes.
| return max(res, key=lambda i: i["created"]) if res else None | ||
|
|
||
|
|
||
| def get_targets_allspec_apred_vers_apstar_id_file_spec(apred_vers: str, apstar_id: str, file_spec: str) -> peewee.ModelSelect: |
There was a problem hiding this comment.
This name is a bit too long and uninformative. I think we could rename this to something like query_allspec_table.
|
|
||
| return vizdb.AllSpec.select().where(vizdb.AllSpec.apred_vers == apred_vers, | ||
| vizdb.AllSpec.apstar_id == apstar_id, | ||
| vizdb.AllSpec.file_spec == file_spec) |
There was a problem hiding this comment.
The allspec table contains data from different releases. I think we want to support querying on a specific release. You can join on the vizdb.Releases model using the allspec.releases_pk.
|
|
||
| """Perform a search for SDSS targets on vizdb.allspace based on apred_vers, apstar_id, file_spec values. | ||
|
|
There was a problem hiding this comment.
Right now this function has 3 required arguments, but users might not always want to query on these parameters, so I think they should be optional. These 3 columns are only for apogee/mwm targets. I think we want to expose other columns from the allspec table that users may want to query on.
I think we should enable queries on the following columns:
allspec_id, release, sdss_phase, observatory, instrument, sdss_id, catalogid, fiberid, ifudsgn, plate, fps_field, plate_or_fps_field, mjd, programname, survey, ra, dec, apogee_id
I don't think we want to necessarily AND all these together. Some would be ORs. Most columns will be strict equals. I think allspec_id we also want to support a LIKE query.
There was a problem hiding this comment.
For RA, Dec columns, we might want to support cone searches with some radius. Users are less likely to specify an exact RA,Dec match.
| # Note for using catalogdb peewee models from sdssdb. | ||
| # | ||
| # Use the below syntax. | ||
| # | ||
| # from sdssdb.peewee.sdss5db import catalogdb | ||
| # catalogdb.Gaia_DR3.ra | ||
| # | ||
| # Do not use the below syntax. | ||
| # | ||
| # from sdssdb.peewee.sdss5db.catalogdb import Gaia_DR3 | ||
| # Gaia_DR3.ra |
There was a problem hiding this comment.
We don't need this comment block.
| return list(targets) | ||
|
|
||
| @router.get( | ||
| "/allspec_apred_vers_apstar_id_file_spec", |
There was a problem hiding this comment.
This endpoint name is too long and convoluted. The name should be simple and easy for a user. I'd suggest allspec. The valis API would then be valis/query/allspec` and a user can easily tell this endpoint is for "querying the allspec table".
| dependencies=[Depends(get_pw_db), Depends(set_auth)], | ||
| ) | ||
| @valis_cache(namespace="valis-query") | ||
| async def get_targets_allspec_apred_vers_apstar_id_file_spec_search(self, |
There was a problem hiding this comment.
This function name is too long and complicated. I'd simplify it.
|
|
||
| @router.get( | ||
| "/allspec_apred_vers_apstar_id_file_spec", | ||
| summary="Perform a search for an allspec target based on the apred_vers, apstar_id, file_spec", |
There was a problem hiding this comment.
| summary="Perform a search for an allspec target based on the apred_vers, apstar_id, file_spec", | |
| summary="Perform a target search on the SDSS allspec table", |
|
|
||
| # throw exception when it's invalid apred_vers, apstar_id, file_spec | ||
| if not targets: | ||
| raise HTTPException(status_code=400, detail=f"Invalid apred_vers {apred_vers}, apstar_id {apstar_id}, file_spec {file_spec}.") |
There was a problem hiding this comment.
| raise HTTPException(status_code=400, detail=f"Invalid apred_vers {apred_vers}, apstar_id {apstar_id}, file_spec {file_spec}.") | |
| raise HTTPException(status_code=400, detail="No targets found in the allspec for given search inputs. Try adjusting your query.") |
| # into a dictionary. | ||
| # The function list() converts the dictionary into a list. | ||
| # The list can then be serialized. | ||
| targets = list(get_targets_allspec_apred_vers_apstar_id_file_spec(apred_vers, apstar_id, file_spec).dicts()) |
There was a problem hiding this comment.
The allspec query may return lots of results. We may want to make this an iterator() and use a StreamingResponse instead. Have you done some performance benchmarks to see how this endpoint performs for large queries? If not, I think we'll want to do some tests comparing this implementation vs iterator/StreamingResponse.
2 participants
No description provided.