-
-
Notifications
You must be signed in to change notification settings - Fork 423
ESA Euclid EUCLIDSWRQ-215 get_datalinks_metadata #3438
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
base: main
Are you sure you want to change the base?
ESA Euclid EUCLIDSWRQ-215 get_datalinks_metadata #3438
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3438 +/- ##
=======================================
Coverage 70.72% 70.72%
=======================================
Files 232 232
Lines 20041 20045 +4
=======================================
+ Hits 14174 14177 +3
- Misses 5867 5868 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Great, thanks @jsaizsantos ! Please let us know if you have further comments, @bsipocz . We are preparing for an internal release at the end of this month, so it would be great if we could have this reviewed and merged by next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jsaizsantos for the PR and welcome to astroquery!
I have some comment for the content here, namely to consider not having the separate method just the new kwarg to get_datalinks
. And maybe calling it something else then option
, maybe extra_options
(I'm not really sold on this idea) or something else that best describes it.
options=options, | ||
verbose=verbose) | ||
|
||
def get_datalinks_metadata(self, ids, *, linking_parameter='SOURCE_ID', verbose=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this as a separate method rather than just documenting the one needs to have use options='METADATA'
? The narrative documentation example below throw me off the track as starting with that one, and based on the method name I expected only metadata rather than it having the same result as using get_datalinks
and some extra metadata.
astroquery/esa/euclid/core.py
Outdated
options : str, optional, default None | ||
To let customize the server behaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
feels very generic; could we be more specific naming this?
Also, I would find it useful to mention what valid values could be for this, from the example below 'METADATA' is one, but how could the user know what else works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, adding a new kwarg is an API change, thus will need to be mentioned in the changelog
astroquery/utils/tap/core.py
Outdated
return user.startswith(f"{user_id}:") and user.count("\\n") == 0 | ||
|
||
def get_datalinks(self, ids, *, linking_parameter=None, verbose=False): | ||
def get_datalinks(self, ids, *, linking_parameter=None, options=None, verbose=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be mentioned in the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @bsipocz, thanks for your comments! I try to explain:
The best option for me, ideally, would have been to just add a new method without changing the signature of the parent get_datalinks
. However, in order to reuse code and avoid copy-paste, I called the existing parent get_datalinks
method from the new get_datalinks_metadata
, and for getting the new behavior from the server, we needed a new argument to that method of the parent TapPlus
class.
You say with good reason that this argument name is generic, and it is on purpose: the idea is that it could be reused in future cases to customize the behavior of the server (as there are several classes extending TapPlus
, for different ESA archives, every one having their requirements) without needing to touch this get_datalinks
method once and again, as it happened this time. The value of the options
argument would tell the exact additional behavior.
I will tackle your suggestions:
- Rename
options
toextra_options
. - Indicate the only valid value for it right now, which is
METADATA
for theEuclid
archive. - Mention the change of signature in the changelog.
Dear @bsipocz: |
Dear Astroquery community:
This is my first PR to this repository. I am a developer working for the Euclid archive at ESAC.
The purpose of this PR is to add a new method
get_datalinks_metadata
to theEuclid
class, to return additional columns on top of the ones returned by the plainget_datalinks
method, which is left untouched as it complies with the IVOA standard.The actual new columns to return are left to the datalinks service of the Euclid archive; currently it is
datalabs_path
, to be used for Datalabs, but two more will come in a new release of the archive (with no further required change on the Astroquery interface):file_name
andhdu_index
.cc @esdc-esac-esa-int