SIAv2 #206
SIAv2 #206
Conversation
|
I'm opening a separate PR to fix the CI. |
|
This looks awesome! I'm going to review this more closely tomorrow, but I like it. I get what you mentioned about should the searchv2 be on some kind of different object. I'm not sure about that but I'll think about it. And I merged the build fix, so you can rebase on top of that and hopefully get your checkmark. |
|
A few thoughts: Why not using inheritance to handle all that duplicate code? Theres also some code for the DALI Values in It also would make sense to check if the registry code still works correctly for sia2 services. |
|
I think I'm with @funbaker , I didn't see that the code was mostly a straight copy from SIAv1 (since that wasn't in the diff), but it seems like just inheriting it might be easier, and it looks like it would get rid of a bunch of code. |
|
The code is not optimized yet but I'll try to avoid duplication whenever possible. I was trying to enhance Another thing to consider is the fact that SIAv2 returns That gets us back to the search function. A common What do you think? |
|
I think it's totally fine to have 2 different search functions, since you really want to be explicit about if you're using v1 or v2 for a search since a given URL might not work with both? I do like trying to reuse the code for the stuff behind the search with inheritance though. Interesting point on the ObsCore results, what are you thinking there? It seems like there's a lot of fields, and most are optional, but I like the idea of wrapping that up a bit, although could you wrap some of the most common ones or the required ones? |
|
In that case it'll be still better to have two different service classes than two different methods on the same class. I like the idea of explicitly implementing the DALI Data Types, as there is already some code for it. A subclass of Edit: The DALI Code is in |
|
SIAP-2.0 states that
(http://www.ivoa.net/documents/SIA/20151223/REC-SIA-2.0-20151223.html#toc34) In a sense an ObsCoreResults would be nice, but based on its TAP usage, with SIAP-2.0 reusing it I'm really happy to see SIAv2 moving on in astropy/pyvo! |
|
@cbanek , @funbaker OK, separate search, separate service class, the SIAQuery class is already different and the SIA record will also be different. Should we just have an |
|
Well, I'm for keeping the interface as simple as possible, hence i spoke out for just one search function. What about a |
Codecov Report
@@ Coverage Diff @@
## master #206 +/- ##
=========================================
Coverage ? 72.08%
=========================================
Files ? 40
Lines ? 4402
Branches ? 0
=========================================
Hits ? 3173
Misses ? 1229
Partials ? 0
Continue to review full report at Codecov.
|
|
@cbanek, @funbaker -Please have a look at the
Again, it's not ready for review yet, just to give you an idea of the direction and ensure I'm on the right track. Please feel free to comment at your earliest convenience so that I know whether I can continue or change direction. Thanks |
|
Apart from a few minor issues i wonder why the properties return a copy. Also i still think it'd be better to have dedicated classes for the DALI Types, submerged with the existing code. Can also be done later, but it wouldn't need the |
Properties are lists because they can have multiple values that SIAv2 will OR in the query. One very simple example is The alternative was to give users access to the properties list and do the parsing and formatting of the values for the request params later before submitting it. What I didn't like with this approach was the fact that parsing would not fail when a property was set (or added) but much latter. I'm not 100% satisfied with the API of the What DALI types are you thinking about? Can you give an example? If it's a better solution I rather do it now than later. |
|
I speak about those DALI Types: http://www.ivoa.net/documents/DALI/20170517/REC-DALI-1.1.html#tth_sEc3 astropy's Quantity's can be used for that, and it should be convertable between votable fields and params (for datalink etc, see also https://github.com/astropy/pyvo/blob/master/pyvo/dal/params.py, this needs some overhaul anyway). |
|
I see, but I can't just wrap my mind around it, in part because I'm not that familiar with all these standards and how they relate to each other. Are these DALI types used internally only or they are part of the module API? For example, I didn't consider necessary to expose a Point or a Circle or a Polygon in the interface when they can be easily expressed with SkyCoord and Quantities. I still have to move (or at least split) ObsCoreRecord. I've started with a I've pushed more changes if you want to have a look (ObsCoreRecord changes not included yet). |
|
They are part of the API, both inernally and externally at different places. Having all the data type related code there would make the query api look less bloated, and it'd possible to write There are already several places (sia2, datalink, soda) where those types are used, and I'm sure there will be more in the future, so it's also a question of duplicate code. |
|
On Thu, Feb 06, 2020 at 09:03:08PM -0800, andamian wrote:
I see, but I can't just wrap my mind around it, in part because I'm
not that familiar with all these standards and how they relate to
each other. Are these DALI types used internally only or they are
part of the module API? For example, I didn't consider necessary to
expose a Point or a Circle or a Polygon in the interface when they
can be easily expressed with SkyCoord and Quantities.
DALI is intended as a collection of common conventions for data
access layer specifications, so whatever is there can be expected to
show up in several standards.
For the xtypes, the story is a bit more sinister (or that is how I
would call it). They're building on what one might call a backdoor
for extending VOTable and define ways to represent geometries,
timestamps, and intervals in VOTables and beyond.
Hence, ideally these extra types (given in the subsections of
http://ivoa.net/documents/DALI/20170517/REC-DALI-1.1.html#tth_sEc3.3)
should, ideally, go into astropy's VOTable support, where they would
presumably be turned into native astropy types where these exist.
And these native astropy types should, IMHO, be used in the
high-level, "pythonic" API to such services.
That's my remote wishlist. My closer-range wishlist derived from
that would look somewhat like this: let tinkerers feed in strings if
they want in a plumbing-level API, and have a translation layer
turning native astropy objects (or pyvo-local abstractions where
astropy doesn't have anything yet) into these strings, which would be
the default API normal people would use.
|
|
Very valuable comments @funbaker & @msdemlei. To get the level of abstraction right, I feel like we need to clarify one thing up front: who do you consider to be the typical user of If it's the former, then it's OK to have other VO abstractions in the API (the This will guide us in future developments. |
|
I'd say both.
If those DALI Objects are constructed in a way which allow tuples of This is already done e.g. here https://github.com/astropy/pyvo/blob/master/pyvo/dal/adhoc.py#L705 |
|
Just giving this another look now and I'm confused - it looks like there's an sia2.py but the names are the same as sia.py. Seems like this could be confusing with the same names. If this is part of the "it's not ready yet" then disregard 😄 . |
|
Now I see what you mean @funbaker. I'll try to rework it so that that code in Soda and other places is not duplicated. |
|
@cbanek - that was on purpose. We are using the same design and same constructs so that for someone that is already familiar with the |
|
On Thu, Feb 13, 2020 at 10:59:58PM -0800, andamian wrote:
Changes are in as promised and I think we are ready for a line by
line review.. Some notes for someone looking at the code just now:
Not sure when I'll have time to squeeze this in, so I'll be grateful
to anyone else having a look. But:
3. obs_publisher_did seemed a bit hard to understand by general
public so I call it `global_id` (it could also be `universal_id`).
If this is a bad idea and in fact the publisher ID makes sense to
the general community I will use a different name.
Uh, no. There's a second identifier in there that should also be
globally unique, obs_creator_did. The distinction is simply that the
creator DID would be assigned by whoever produces the dataset and it
would go with it as different publishers publish it.
While I've not seen too many of them in the wild -- try
```
select top 30 *
from ivoa.obscore
where obs_collection='TMAP'
```
on http://dc.g-vo.org/tap if you want to see some --, they're still
there and would arguably better deserve the title "global id", as,
where given, it would actually identify a dataset as such, rather
than "something published here". For instance, once creators
actually assign them, you could use this to weed out duplicates
from different datacenters that mirror data collections.
So, I'd argue it should be clear from the attribute name that
obs_publisher_did is an identifier assigned by the publisher. I give
you the "obs_" at the start isn't pretty, but frankly I'd rather not
abstract obscore names away without a strong reason, because after
all I'd expect many users would write queries against these columns
and thus would be familiar with their names.
If you still absolutely detest the name obs_publisher_did (for which
I would have some sympathy if it weren't part of the "obscore UI"), a
more helpful name might be publisher_assigned_id, or perhaps even
publisher_assigned_globally_unique_id. But I don't think anyone
would want to type that latter thing.
|
|
Made some comment - not requested changes at this point, because i'll do another iteration anyway and I'm not 100% sure if all points are valid. |
|
@msdemlei - sold. Thank you for the nice explanation, some of which I've tried to capture in the docs. I've re-named it |
|
Well, LGTM when CI is green (unless someone else has something to say). |
|
I have no idea why the tests are failing on Travis. The data directory and the capabilities file are there. Any idea? |
|
@andamian probably because the test data is not defined in |
|
Yeah - thanks. Figured it out eventually. Confusing because it works fine locally. What do you think should we do about the outstanding issues: |
|
I'd say proper types, with those tuples to be passes into the constructor, so that they can be magically autoconstructed when a tuple is passed. I did a similar thing in the other interfaces. |
|
By automagically you mean something like: |
|
I'm really stuck with the sphinx error. Don't even know what where to look. |
Something like this, yep.
There's no dedicated classes, just some code in About sphinx: Looks like a warning about a docstring which is converted into an error. |
Those classes don't deal with Quantities, Units and don't validate the inputs. I'll need to move the validation code that I have for
I know. Just can't figure out exactly which docstring it's referring to. |
|
@funbaker - FYI, for using the Circle and Polygon classes, I will need to expand their usage. Instantiating one of these classes might result in I don't see them being used at the moment, so no danger in breaking something I hope. The other thing is that I can't tell what the param input is supposed to be because is not list or tuple. |
|
I took a look at your sphinx issue and debugged it. Looks like it was an indentation issue. This is kind of cute to not repeat the long docstring, but it's really confusing to me. In the other place where you use it, the indentation is different. Looking at the docs themselves, we are only showing the doc for the search function, we aren't including any other functions, as they would have the same names as the normal SIA classes... Here's a gist that I cooked up that doesn't error, but it doesn't render properly either, since the rendering of what is a parameter or if it's the text of the previous one is the indentation level: My simple suggestion would just be to copy the comment block and try not to do some string manipulation on it when it's imported, then the indentation will be really much more obvious. |
|
@funbaker & @cbanek - thank you for the code reviews and approvals. However, I am not comfortable to go ahead with the merge and release, at least not until I get more feedback on the |
|
@funbaker & @cbanek pushed another commit. Attributes in I have another work in progress in |
|
On Thu, Feb 20, 2020 at 02:41:35PM -0800, andamian wrote:
@funbaker & @cbanek pushed another commit. Attributes in
`ObsCoreMetadata` match the names in `ObsCore`. I've also fixed the
tests so no we can run `remote-data` ones. After your blessing, can
we merge and possibly release?
Well, I personally could have done without the obs_, too, but it's
probably better not to mess with names people might know from
ivoa.obscore. So, you absolutely have my blessing for
ObsCoreMetadata.
Just so I can later say "told you so", I'll repeat my reservations
about the design decision to expose the POS argument that I believe
is a misfeature of SIAv2; I'd have implemented circle and polygon as
in SODA. In my practice, polymorphic geometries have been a lot more
pain than they've been gain.
But from the fact that I can't be bothered to make a PR to introduce
circle and polygon arguments you can probably guess that I'm not
convinced myself that pyvo is the place to fix SIAv2. And as long as
we don't know how astropy will eventually model such geometries (or
do we?), pos probably wins on the side of being more supple when
adapting to whatever astropy might be throwing at us.
|
#205