Add source_value to the describe function in regtap

[ManonMarchand]

Hello pyvo,

Why the PR

I'd really like if we could add the source_value to the describe output for regtap if that's ok with you. For now, I added it to the non-verbose output, but we could also limit that to verbose.

What's done here

It's a bit messy:

• first commit just corrects a few typos in the docs
• then add the source to describe function
• then regenerate multiinterface.xml in the tests data because source_value was not there. This required to adapt the datalink standard new version number

Thanks!

[codecov]

Codecov Report

Merging #492 (62efe9d) into main (75bcb73) will increase coverage by 0.06%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   80.01%   80.07%   +0.06%     
==========================================
  Files          52       52              
  Lines        6039     6059      +20     
==========================================
+ Hits         4832     4852      +20     
  Misses       1207     1207              

Files	Coverage Δ
pyvo/registry/regtap.py	84.04% <100.00%> (+1.21%)	⬆️
pyvo/registry/rtcons.py	97.83% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ManonMarchand]

@gilleslandais proposed that we also add the DOI in this function. That'd require me to add rr.alt_identifier to the joined_tables in build_regtap_query I guess.

(here)

I'll add that if no one is against the idea. Switching to draft cause I won't have time over the weekend.

[ManonMarchand]

The latest version here adds source_value and alt_identifier to the output of the verbose version of describe.
It looks like this:

from pyvo import registry
catalogue_ivoid = f"ivo://CDS.VizieR/J/AJ/157/229"
voresource = registry.search(ivoid=catalogue_ivoid)[0]
voresource.describe(verbose=True)

CALSPEC: WFC3 infrared grism spectrophotometry
Short Name: J/AJ/157/229
IVOA Identifier: ivo://cds.vizier/j/aj/157/229
Access modes: conesearch, tap#aux, web
Multi-capability service -- use get_service()

The collections of spectral energy distributions (SEDs) in the Hubble Space
Telescope (HST) CALSPEC database are augmented by 19 infrared (IR) SEDs from
Wide Field Camera 3 (WFC3) IR grism spectra. Together, the two IR grisms, G102
and G141, cover the 0.8-1.7 {mu}m range with resolutions of R=200 and 150,
respectively. These new WFC3 SEDs overlap existing CALSPEC Space Telescope
Imaging Spectrograph (STIS) standard star flux distributions at 0.8-1 {mu}m
with agreement to ~<1%. Some CALSPEC standards already have near-IR camera and
multi-object spectrogragh (NICMOS) SEDs; but in their overlap region at
0.8-1.7 {mu}m, the WFC3 data have better wavelength accuracy, better spectral
resolution, better repeatability, and, consequently, better flux distributions
of ~1% accuracy in our CALSPEC absolute flux SEDs versus ~2% for NICMOS. With
the improved SEDs in the WFC3 range, the modeled extrapolations to 32 {mu}m
for the James Webb Space Telescope flux standards begin to lose precision
longward of the 1.7 {mu}m WFC3 limit, instead of at the 1.0-{mu}m-long
wavelength limit for STIS. For example, the extrapolated IR flux longward of
1.7 {mu}m for 1808347 increases by ~1% for the model fit to the data with
WFC3, instead of just to the STIS SED alone.

Subjects: J/AJ/157/229
Waveband Coverage: infrared

 More info:
Reference url: https://cdsarc.cds.unistra.fr/viz-bin/cat/J/AJ/157/229
Alternative identifier: doi:10.26093/cds/vizier.51570229
Source: 2019AJ....157..229B

[msdemlei]

Thanks for fixing all the documentation typos!

Also, including alt_identifier in our resource records as such is good idea, and the source should always have been in the description.

So, from my point of view, this is mergeable, except that perhaps the commit messages could be just a bit less terse. With commit messages you'd like to see in the main history, we could merge the PR by rebasing, I think.

But on the question of how to merge, I'd like to defer to @bsipocz.

[gilleslandais]

Thank you Manon,
I would like to suggest other metadata to complete the descrtiption like authors, publication date.
Have a look at https://www.ivoa.net/documents/DataOrigin/, section 6 « Data Origin in Registry ».

I propose to add a subart of this list :

• authors : in particular the first author (in reg. : res_role.role_name )
  Other authors could be addedd too if the list is not too long (or could be truncated with « et al. »?)
• creation date (in reg. : resource.created)

in option :

• referenceURL : the landing page (in reg. Resource.reference_url)
• publisher (in reg. : res_role.role_name)
• contact : (in reg. : res_role.contact)
• rights : do we have to propose right in VO context ? (in reg. : resource.right or resource.right_uri) -

gilles

[ManonMarchand]

Thanks for the review, I accepted the two suggestions (about webbrowser and More info:) and I tried writting longer commit messages. If this is not long enough, feel free to edit as much as needed.

I switched back to draft because @gilleslandais wanted to comment before merging this

[ManonMarchand]

Thank you Manon, I would like to suggest other metadata to complete the descrtiption like authors, publication date. Have a look at https://www.ivoa.net/documents/DataOrigin/, section 6 « Data Origin in Registry ».

I propose to add a subart of this list :

* authors : in particular the first author  (in reg. : res_role.role_name )

Is there a way to know which of the persons with role_name = creator is the first author?

If not, there is the pyvo.registry.regtap.RegistryResource.creators property that contains a list of the authors. Maybe a suggestion could be to add the authors to the description if there is a reasonable number (~5) and to tell users to check the attribute themselves if there is more (a bit like what is done for the Multi-capability services)?

  Other authors could be addedd too if the list is not too long (or could be truncated with « et al. »?)

* creation date  (in reg. : resource.created)

We could have created and updated here. Would be useful for people who'd like to know if they have the latest version of a resource.

* referenceURL : the landing page (in reg. Resource.reference_url)

This one is given by the More info: line. I guess there is a debate on what to call this

* publisher  (in reg. : res_role.role_name)

Voting in favor.

* contact : (in reg. : res_role.contact)

I think we can leave this one with the get_contact method. It's well documented.

* rights :  do we have to propose right in VO context ? (in reg. :  resource.right or resource.right_uri) -

Don't know. Most of the entries don't have a rights value.

[gilleslandais]

about creation date/update date.
I think that creation date is more important.
For me, "update" sounds to be a metadata that like the version (which exists in the registry) is used for reproducibility purposes (I don't know if the description is in this scope ?)

[ManonMarchand]

For the authors order, got it: creator_seq in in the right order

[msdemlei]

On Mon, Oct 02, 2023 at 05:36:22AM -0700, Manon Marchand wrote: Is there a way to know which of the persons with `role_name = creator` is the first author?

No-ish. From rr.res_role you cannot because of SQL semantics (and because RegTAP doesn't add something like an element_index column). For display purposes, however, there's the rr.resource.creator_seq you discovered as RegistryResource.creators. That preserves the authors in sequence and tries to present them in a reasonable format. I'd say that if you want to show creator information, that's where you'd look, perhaps cutting off at 80 characters or something with an ellipsis if you're worried about endless author lists. RegTAP requires "; " as a separator here, so you could try and get more fancy as in

> Other authors could be addedd too if the list is not too long (or could be truncated with « et al. »?)

But really: I wouldn't. Publishers dump all kinds of horrors into creator, so trying to parse creator_seq is going to be a world of pain at this point.

> * contact : (in reg. : res_role.contact) I think we can leave this one with the `get_contact` method. It's well documented.

Agreed; and it would make the normal discovery query significantly more complicated. I'd say we can leave the more exotic use cases for extra methods like these (that then may incur extra queries, too).

> * rights : do we have to propose right in VO context ? (in > reg. : resource.right or resource.right_uri) - Don't know. Most of the entries don't have a `rights` value.

At this point, I'd argue rights information is too inhomogeneous to be widely useful. But if we show something, it should be the contents of resource.rights -- that's where human-readable information on usage restrictions and the like should be kept. rights_uri is intended to be machine-readable; I don't think it has a place in .describe().

[ManonMarchand]

I just saw that short name is printed twice (after Short_name: and after Subjects:)

[bsipocz]

TL,DR: this can be merged whenever is ready and moved out from draft status and Markus, or the other maintainers are still happy with it. (There have been a couple of commits and a discussion since the approval, so I would not like to be the one overriding or deciding. From the plumbing POV, it all looks good to me).

The commits look reasonable to me, multi-lines are all good. Maybe it could have been better to submit the typo fixes separately, e.g. that way they could have landed in 1.4.3, in case a bugfix will be released before 1.5. But it's no big deal either way.

As for the commit messages, I really like that you use prefixes, too. I personally like the more standard acronyms that e.g. numpy documents here (https://numpy.org/devdocs/dev/development_workflow.html#writing-the-commit-message), but honestly it's really a tiny nuance, and I don't see we will ever require to follow one way or the other.

[ManonMarchand]

Hi all,

TL; D:R If the approval still stands, you can merge 🙂

Comments:

Following the comments, I added created, updated and rights to the registry.regtap.RegistryResource attributes cause it wasn't costing lots (already in the resource table and interested users can read the value if they need it) but I don't think I'll address the other comments here. Maybe open an issue and we debate there before changing more things?

PS: for the commit messages, my resolution for the university new year was to try and follow https://www.conventionalcommits.org/en/v1.0.0/ when repos have no guidelines. This PR was my first time experimenting with that, sorry for the stir it caused

[msdemlei]

Here are a few additional comments -- but feel free to discard them and merge as-is.

[msdemlei]

On Wed, Oct 04, 2023 at 04:40:01AM -0700, Manon Marchand wrote: @ManonMarchand commented on this pull request. + @Property + def rights(self): + """Rights information about the content of the resource. + + This information is often incomplete in the registry, you + might get more information with the ``reference_url``. I don't get what you mean here

Sorry, I was typing sed commands :-). I just suggested to write "more information at ``reference_url``" instead of the construct with "with the". But that's purely stylistic, and not being a native speaker myself I might even be wrong.

[ManonMarchand]

Ah sure! I asked a native speaker and he advised 'at the reference_url', the change is now applied 🙂

[bsipocz]

Thank you @ManonMarchand for the PR and @msdemlei for the review!