Add datalink original row #559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msdemlei
force-pushed
the
add-datalink-original_row
branch
from
ceeda65
to
a8e7643
Compare
msdemlei
force-pushed
the
add-datalink-original_row
branch
from
a8e7643
to
8ee5ba2
Compare
msdemlei
force-pushed
the
add-datalink-original_row
branch
from
8ee5ba2
to
37c09ad
Compare
bsipocz
reviewed
Jun 28, 2024
docs/dal/index.rst
Outdated
| obscore or SIAP service, where the media type is | ||
| application/x-votable;content=datalink –, one can build a | ||
| DatalinkResults using | ||
| :py:meth:`~pyvo.adhoc.DatalinkResults.from_result_url`: |
There was a problem hiding this comment.
This is not the correct namespace, thus the sphinx error.
Suggested change
| :py:meth:`~pyvo.adhoc.DatalinkResults.from_result_url`: | |
| :py:meth:`~pyvo.dal.adhoc.DatalinkResults.from_result_url`: |
bsipocz
reviewed
Jun 28, 2024
docs/dal/index.rst
Outdated
| .. doctest-remote-data:: | ||
| >>> rows = vo.dal.TAPService("http://dc.g-vo.org/tap" | ||
| ... ).run_sync("select top 5 * from califadr3.cubes order by califaid") | ||
| >>> for dl in rows.iter_datalinks(): |
There was a problem hiding this comment.
This example runs into the DALFormattingError, that is very similar to #361, thus it fails the test.
(Technically it's a warning, so we can deal with it if necessary, but it would be nicer to fix it instead, if possible)
msdemlei
force-pushed
the
add-datalink-original_row
branch
from
37c09ad
to
cb921a6
Compare
|
Sorry for another force-push, and one that reverses the history on top. On the other hand, this has now a passing and reasonably fast docs/dal/index.rst. So... if the CI passes, could you have another look, @bsipocz? |
|
On Thu, Jun 27, 2024 at 06:26:49PM -0700, Brigitta Sipőcz wrote:
This example runs into the `DALFormattingError`, that is very similar to #361, thus it fails the test.
(Technically it's a warning, so we can deal with it if necessary, but it would be nicer to fix it instead, if possible)
Oh, I forgot to comment on this: I've put an IGNORE_WARNINGS there
now, because while Tom has promised to do the astropy fix RSN, it
will still be a while until the fix will arrive in our test
systems, let alone in Debian, which is where my doctests run...
|
msdemlei
force-pushed
the
add-datalink-original_row
branch
from
cb921a6
to
6c8907f
Compare
bsipocz
reviewed
Jul 1, 2024
pyvo/dal/adhoc.py
Outdated
Comment on lines
656
to
657
| res = super(DatalinkResults, cls).from_result_url( | ||
| result_url, session=session) |
There was a problem hiding this comment.
can be written in one line, maybe more readable that way
Suggested change
| res = super(DatalinkResults, cls).from_result_url( | |
| result_url, session=session) | |
| res = super().from_result_url(result_url, session=session) |
| @@ -597,7 +619,7 @@ def clone_byid(self, id): | |||
| for x in copy_tb.resources: | |||
| if x.ID and x.ID not in referenced_serviced: | |||
| copy_tb.resources.remove(x) | |||
| return DatalinkResults(copy_tb) | |||
| return DatalinkResults(copy_tb, original_row=original_row) | |||
There was a problem hiding this comment.
shouldn't original row be a copy as well, from whatever is already set in the class, in self.original_row?
|
On Mon, Jul 01, 2024 at 04:44:32PM -0700, Brigitta Sipőcz wrote:
@bsipocz commented on this pull request.
Minor comments only, except that `original_row=None` doesn't feel
right in `clone_byid` as a default, shouldn't that be a copy as
well from the object?
No, I'm pretty sure we should not copy original_row despite the
"clone" in the name. The reason is that clone_byid is passed an id;
in other words, in all likelihood the new DatalinkResults will
corresond to a different "thing", and hence, if there was an original
row, to a different original row.
Now, I am fairly adamant that it is more important to avoid wrong
data than to have some additional possibly correct data (see also
the Zen of Python: "In the face of ambiguity, refuse the temptation
to guess"). So, for reasons of defensive programming, I think I'll
stubbornly insist on None here.
> + def __init__(self, *args, **kwargs):
+ self.original_row = kwargs.pop("original_row", None)
+ super().__init__(*args, **kwargs)
I don't think this is necessary as you already do the same in the
`DatalinkResultsMixin` base class.
Ummm... sorry, I don't follow here: Are you referring to the __init__
method? But the Mixin doesn't have a constructor (as I, frankly,
would expect from a Mixin)? The upcall goes to DALResults, and that
won't touch original_row.
Am I missing something fundamental?
> +.. doctest::
+
No need for this, everything with `>>>` are picked up as doctest
Hmyeah... can we still keep it to make things simpler for my
zero-tooling doctest profiler? [cf. my plea in #567]
> + res = super(DatalinkResults, cls).from_result_url(
+ result_url, session=session)
can be written in one line, maybe more readable that way
```suggestion
res = super().from_result_url(result_url, session=session)
```
Shiver my timbers. I indeed had not realised that the argument-less
super is smart enough to even work in a class method, but it
apparently does. Wow. One of these days I'll try and understand how
it does that :-)
Anyway: changed in commit 5360cff.
> @@ -597,7 +619,7 @@ def clone_byid(self, id):
for x in copy_tb.resources:
if x.ID and x.ID not in referenced_serviced:
copy_tb.resources.remove(x)
- return DatalinkResults(copy_tb)
+ return DatalinkResults(copy_tb, original_row=original_row)
shouldn't original row be a copy as well, from whatever is already
set in the class, in `self.original_row`?
First off, it is my conviction that the wider vicinity of that
code needs a thorough review anyway and do (a lot) *less* copying (I
have bug #537 to prove it).
Your point, however, is a good observation: Do we want to comment on
what's supposed to happen if someone changes original_row (regardless
of whether it comes in through clone_byid or through some other
means)?
Me... well, I can't think of a case where it would actually be useful
if datalink client code modifies the table the datalinks come from.
On the other hand, I don't think I want to copy (or materialise) all
these rows just in case. So, in commit 5360cff I have added a bit
of language to the docs to the effect that you're not supposed to
rely on any behaviour when you muck around with original_row.
Thanks for the review!
|
Using this opportunity to speed up the doctest by using smaller/faster services. It's now running in 10 seconds on my box in my neck of the net, which sounds a lot more reasonable than what was there before.
It also does some minor improvements to the documentation of how to use datalink. But that part could really use a lot more love...
msdemlei
force-pushed
the
add-datalink-original_row
branch
from
5360cff
to
84d48e4
Compare
|
my bad about the |
bsipocz
approved these changes
Jul 2, 2024
|
Thanks @msdemlei! |
andamian
reviewed
Jul 2, 2024
|
|
||
| .. remove skip once https://github.com/astropy/pyvo/issues/361 is fixed | ||
| .. doctest-skip:: | ||
| Datalink lets operators associate multiple artefacts with a dataset. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt to fix bug #542. It's not particularly pretty, but I don't think becoming much cleverer here is worth it.
I'm marking this as a draft while I'm still working out whether we should pass around a few extra original_row-s.