Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
pllim
left a comment
There was a problem hiding this comment.
Hello and thanks!
At first glance, the Sphinx docstring and user doc need some clean up (e.g., remove extra indentation, blank line after colon before listing, whitespace after >>>, and so on). Please use existing doc in the repo as reference.
Your new module also need an __all__ that only contains public API.
If you have a downstream PR ready to use this, please cross link here also.
ba07eaa to
cda8522
Compare
|
There are warnings in your doc build that must be addressed.
|
9c07312 to
d2011a7
Compare
I removed warnings except 2 alerts that I have not managed to delete : /home/docs/checkouts/readthedocs.org/user_builds/astropy/checkouts/17839/docs/io/votable/dataorigin.rst:5: WARNING: duplicate label astropy-io-votable-dataorigin, other instance in /home/docs/checkouts/readthedocs.org/user_builds/astropy/checkouts/17839/docs/io/votable/dataorigin.rst What is wrong? |
d2011a7 to
a4901fe
Compare
pllim
left a comment
There was a problem hiding this comment.
I think you have to add dataorigin API doc to docs/io/votable/ref_api.rst.
These are mostly formatting comments. I will see if I can make sense of the remaining RTD warnings in a separate thread.
pllim
left a comment
There was a problem hiding this comment.
WARNING: py:obj reference target not found: astropy.io.votable.dataorigin
I think this will be fixed after your addition to ref_api.rst.
ff02637 to
10029d6
Compare
|
Hi, |
tomdonaldson
left a comment
There was a problem hiding this comment.
I left a few non-blocker comments, but overall this looks pretty good. I leave it to @bsipocz to add some more comments and make changes as needed to meet the code freeze.
| dores = dataorigin.extract_data_origin(vot.resources[0]) | ||
| dot = dataorigin.extract_data_origin(vot.resources[0].tables[0]) |
There was a problem hiding this comment.
Are these used anywhere?
| Exception | ||
| input type not managed | ||
| """ | ||
| data_origin = DataOrigin() |
There was a problem hiding this comment.
Possible refactor: Have each of these types implement extract_data_origin_info() (or similar name), and have them inherit from a common ancestor which specifies that abstract method.
| from astropy.table import Column, Table | ||
|
|
||
|
|
||
| def __generate_votable_test(): |
There was a problem hiding this comment.
In addition to the programmatic construction of a VOTable here, it would be nice to have at least one actual VOTable added to the data area to be parsed and tested here. This would do a deeper tests than what is currently happening in the doc tests.
bsipocz
left a comment
There was a problem hiding this comment.
Some comments, the two major ones that I think needs to be fixed prior merging are changing the type of exceptions being raised, and the other is to fix up the narrative docs with using more remote-data directives, etc.
astropy/io/votable/dataorigin.py
Outdated
| astropy.io.votable.tree.Table, | ||
| ), | ||
| ): | ||
| raise Exception("Bad type of vot_element") |
There was a problem hiding this comment.
This has to be a more specific exception, basically we must never raise a plain generic Exception
astropy/io/votable/dataorigin.py
Outdated
|
|
||
| elif info_name in DATAORIGIN_QUERY_INFO: | ||
| if not isinstance(vot_element, astropy.io.votable.tree.VOTableFile): | ||
| raise Exception( |
astropy/io/votable/dataorigin.py
Outdated
| vot_element.infos.extend([new_info]) | ||
| return | ||
|
|
||
| raise Exception("Unknown DataOrigin info name") |
| dores = dataorigin.extract_data_origin(vot.resources[0]) | ||
| dot = dataorigin.extract_data_origin(vot.resources[0].tables[0]) |
There was a problem hiding this comment.
just wondering, how do unused variables pass codestyle, is that expected @pllim?
There was a problem hiding this comment.
We should add a few more asserts here anyway, e.g.
assert dores.origin[0].creator == do.origin[0].creator
| for dseto in do: | ||
| pass |
There was a problem hiding this comment.
Are we just testing if this is iterable?
| do = dataorigin.extract_data_origin(vot) | ||
|
|
||
| dores = dataorigin.extract_data_origin(vot.resources[0]) | ||
| dot = dataorigin.extract_data_origin(vot.resources[0].tables[0]) |
There was a problem hiding this comment.
Is this supposed to extract nothing?
In [27]: print(dot)
In [28]: print(dot.origin)
[]
In [29]: print(dot.query)
In [30]:
| assert len(do.origin) == 1 and len(do.origin[0].creator) == 2 | ||
| assert do.origin[0].creator[0] == __TEST_CREATOR1_NAME | ||
| assert len(str(do)) > 1 | ||
| assert (do.origin[0].get_votable_element()) != None |
There was a problem hiding this comment.
!= None should not pass codestyle again, right @pllim
docs/io/votable/dataorigin.rst
Outdated
| >>> # get DataOrigin with the description of each INFO | ||
| >>> for dataset_origin in data_origin.origin: # doctest: +REMOTE_DATA | ||
| ... for info in dataset_origin.infos: # doctest: +REMOTE_DATA | ||
| ... print(f"{info.name}: {info.value} ({info.content})") # doctest: +REMOTE_DATA +IGNORE_OUTPUT |
There was a problem hiding this comment.
we should try and drop +IGNORE_OUTPUT in some of these tests, so they can test more than for basic crashes. E.g. I feel this one would be a safe one to drop (and ultimately once more tests are added in the module itself (see Tom's comment), then we could tune back on doctesting if it turns out to be too flaky
docs/io/votable/dataorigin.rst
Outdated
| .. code-block:: python | ||
|
|
||
| # get the Title retrieved in Element | ||
| >> origin = data_origin.origin[0] # doctest: +REMOTE_DATA | ||
| >> vo_elt = origin.get_votable_element() # doctest: +REMOTE_DATA | ||
| >> title = vo_elt.description if vo_elt else "" # doctest: +REMOTE_DATA | ||
| >> print(f"APA: {','.join(origin.creator)} ({origin.publication_date[0]}). {title} [Dataset]. {data_origin.query.publisher}. {origin.citation[0]}") # doctest: +REMOTE_DATA +IGNORE_OUTPUT | ||
| APA: Hong K. (2024-11-06). Period variations of 32 contact binaries (Hong+, 2024) [Dataset]. CDS. doi:10.26093/cds/vizier.51670018 |
There was a problem hiding this comment.
generic comment: drop the code-block directive and use remote-data instead, that would cleanup all the lines a little bit
| @@ -0,0 +1,514 @@ | |||
| # Licensed under a 3-clause BSD style license - see LICENSE.rst | |||
|
|
|||
| """Extract Data Origin in VOTable | |||
There was a problem hiding this comment.
This doesn't show up anywhere in the docs, so I would recommend ditching it altogether in favour of narrative documentation and docstrings in the classes/functions.
|
This also needs a rebase to fix up the docs and what's new, I'll start with that along with a patch for the docs addressing the comments above. |
|
@pllim - could you have a look at the raised exception types here, do they make sense to you or shall change them to something else? |
At a glance, they look fine to me. I restarted RTD. 🤞 |
|
RTD failure is related, I'm afraid something gone wrong during my rebase. I'm trying to fix it locally and then push a fix for it. |
|
Thank you so much @gilleslandais for this PR and also thank you for your patience. I'm sorry that we ended up hijacking the PR and addressing our own review comments, but I hope you don't find any of the ones I fixed up controversial. I'll open follow-up issues for the two remaining items from Tom's review, neither of these are release blockers but would be nice to haves. |
Hello,
The pull request includes a simple API to extract basic provenance information from VOTAble header.
Metadata are described in the IVOA Note https://ivoa.net/documents/DataOrigin/index.html (@gilleslandais, @augustfly, @msdemlei, @rsav)