Skip to content
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

Update SPARQL in functions.py #47

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dr-shorthair
Copy link

Suppress references to unused namespaces and deprecated predicates

Suppress references to unused namespaces and deprecated predicates
@dr-shorthair dr-shorthair added bug Something isn't working enhancement New feature or request labels Mar 11, 2020
Copy link
Contributor

@benjaminleighton benjaminleighton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start. We will need to build the new cache and test this code pretty thoroughly before we can merge it.

@dr-shorthair
Copy link
Author

Yes I wonder if qb4st:crs --> geox:inCRS may have fallen through the cracks.
I would expect everything else to be OK.

rdf:predicate rdf:type ;
rdf:object prov:Location .
} .
# UNION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My best guess as to why this was here in the first place is because "overlaps" objects are treated as geo:Features but we want to exclude them from being returned as locations. I'm not sure though, @ashleysommer ?

Copy link
Author

@dr-shorthair dr-shorthair Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these computed ahead and cached? If so, I would prefer to see an explicit feature-type for overlaps. Maybe loci:Overlap ? Then these can be explicitly excluded, rather than borrowing an unrelated feature-type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree it shouldn't be a feature but I can't remember all the reasoning around it from our older discussions. I believe we can actually get rid of overlaps objects altogether because all that information should be elsewhere but @ashleysommer has more history and probably more insight on this. We need to break up scope here as well. It would be good to merge whatever is the minimum set of changes to support the new LDAPIs and deal with larger refactors, like overlaps, later.

functions.py Outdated
PREFIX geox: <http://linked.data.gov.au/def/geox#>
PREFIX qb4st: <http://www.w3.org/ns/qb4st/>
# PREFIX qb4st: <http://www.w3.org/ns/qb4st/>
PREFIX epsg: <http://www.opengis.net/def/crs/EPSG/0/>
PREFIX dt: <http://linked.data.gov.au/def/datatype/>
SELECT <SELECTS>
WHERE {
{
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this bracket might not be closed and might need commenting

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which bracket? there is nothing in the change.

functions.py Outdated
rdf:predicate geox:transitiveSfOverlap;
rdf:object ?o .
} UNION {
# ?s1 rdf:subject <URI> ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can take this out until we've rebuilt the linksets and that is beyond the scope of the LDAPI refactors currently underway. AFAIK we use geox:transitiveSfOverlap still and can't depend on geo:sfOverlaps yet

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets verify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in tests with excelerator use cases, we either have to:
a) change the linksets and/or triples in the cache so that this API endpoint works with your proposed query @dr-shorthair
or
b) defer removing the union statement (i.e. keep it) so it works with the current linkset triples

b) is more expedient; but a) is probably what we should do in the longer term

functions.py Outdated
rdf:predicate geox:transitiveSfOverlap;
rdf:object ?o .
} UNION {
# ?s1 rdf:subject <URI> ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it is the bracket above this comment I think it also needs commenting. Not sure though it just looks like an extra line needs commenting.

rdf:predicate rdf:type ;
rdf:object prov:Location .
} .
# UNION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree it shouldn't be a feature but I can't remember all the reasoning around it from our older discussions. I believe we can actually get rid of overlaps objects altogether because all that information should be elsewhere but @ashleysommer has more history and probably more insight on this. We need to break up scope here as well. It would be good to merge whatever is the minimum set of changes to support the new LDAPIs and deal with larger refactors, like overlaps, later.

@dr-shorthair
Copy link
Author

So far I have only looked at one file: functions.py
Is there SPARQL anywhere else (I can't find any).

@jyucsiro
Copy link
Contributor

@dr-shorthair i think all the SPARQL in the loci-integration-api is in functions.py

@dr-shorthair
Copy link
Author

dr-shorthair commented Apr 22, 2020

I've simplified the geo:sfOverlaps query to match all sub-properties as well. See lines 645-655.
This should catch the geox:transitiveSfOverlap cases now, and ignore them later, without actually mentioning them.

PREFIX epsg: <http://www.opengis.net/def/crs/EPSG/0/>
PREFIX dt: <http://linked.data.gov.au/def/datatype/>
SELECT <SELECTS>
WHERE {
?p rdfs:subPropertyOf* geo:sfOverlaps .
Copy link
Contributor

@jyucsiro jyucsiro Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dr-shorthair
Copy link
Author

Yes - it is a very small ontology. But we'll have to make sure that the deprecated elements are back in there. I'll let you know when that's done.

@dr-shorthair
Copy link
Author

Shoudl be good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants