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

Get list of unique products #3096

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Conversation

snbianco
Copy link
Contributor

@snbianco snbianco commented Sep 20, 2024

Added implementation, a test, and documentation for a new function (Observations.get_unique_product_list). Given observations, the function returns a list of associated data products, each with a unique data URI. This is essentially a wrapper to Observations.get_product_list that removes duplicate rows before returning.

Because get_product_list is a query method, it should be async and return a response that is then parsed into a Table. Because we need the actual result Table to remove the duplicates, I wasn't able to add this functionality as a parameter to the original function. It's possible that the Mast.Caom.Products service could filter out duplicates at some point, but I think this is the best solution for now.

@snbianco
Copy link
Contributor Author

snbianco commented Sep 20, 2024

@dr-rodriguez @astrojimig

@snbianco snbianco marked this pull request as ready for review September 20, 2024 12:51
products = self.get_product_list(observations)
unique_products = self._remove_duplicate_products(products)
if len(unique_products) < len(products):
log.info("To return all products, use `Observations.get_product_list`")

Choose a reason for hiding this comment

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

This log message could be slightly more informative if you include the numbers, for example "{len(products) - len(unique_products)} duplicate products removed; To return all products, use Observations.get_product_list"

Choose a reason for hiding this comment

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

Ah never mind, the other log message captures that info! You're way ahead of me :)

@astrojimig
Copy link

This looks good! Thanks @snbianco !

@bsipocz bsipocz added the mast label Sep 25, 2024
@bsipocz bsipocz added this to the v0.4.8 milestone Sep 25, 2024
Copy link
Member

@bsipocz bsipocz 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, thanks! I would suggest a follow-up docs cleanup PR that would make the links better.

"""
Given a "Product Group Id" (column name obsid), returns a list of associated data products with
unique dataURIs. Note that obsid is NOT the same as obs_id, and inputting obs_id values will result in
an error. See column documentation `here <https://masttest.stsci.edu/api/v0/_productsfields.html>`__.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking, but it would be nice to avoid (and also clean up) these types of here links. Some reading in the topics: https://heyoka.medium.com/dont-use-click-here-f32f445d1021

We have a bunch of these already, mostly in the mast module, so I don't hold merging up, but I would really encourage a cleanup.

@bsipocz bsipocz merged commit 1bdf695 into astropy:main Sep 25, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants