Skip to content

Conversation

jyucsiro
Copy link
Contributor

Adding test case which loads CDL examples in as RDF into rdflib memory store and executes a bunch of SPARQL queries.

Refer to issue #70

@marqh
Copy link
Member

marqh commented Oct 2, 2017

Hi @jyucsiro

there are a lot of commits on this PR; looking at the commit IDs it looks like this includes changes form #53 and #56
is that a correct interpretation?
I think I've got behind on the code reviewing for this Repo, which has clearly partly lead to this
are the PRs #53 and #56 now superceded by this PR?

Does this contain all the elements from those pieces of work that you want included?

thank you
mark

data:

temp =
_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _,
Copy link
Member

Choose a reason for hiding this comment

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

these 'missing data' elements are not required, ncgen will fill these in for us when it runs, so they can be left out

data:

temp =
_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _,
Copy link
Member

Choose a reason for hiding this comment

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

these 'missing data' elements are not required, ncgen will fill these in for us when it runs, so they can be left out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will do


def check_uri(self, uri):
result = False
#print("Checking uri: " + uri)
Copy link
Member

Choose a reason for hiding this comment

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

please may we remove this commented out print?

for attr in self.attrs:
objs = self.attrs[attr]
if(isinstance(objs, np.ndarray)):
#print("Found np.ndarray")
Copy link
Member

Choose a reason for hiding this comment

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

please may we remove these commented out prints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will do.

f.close()

def load_netcdf(afilepath, uri=None):
def load_netcdf(afilepath, uri=None, baseuri=None):
Copy link
Member

Choose a reason for hiding this comment

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

I am wary that there might be confusion in our code regarding uri and baseuri

what are the differences in use between these two? Can we use the same input for all cases, to guard against confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had understood the var 'uri' to be the root_uri of the graph. so there was a gap in an ability to specify a default prefix uri for entities in the graph that weren't qualified, which was the intention of 'baseuri'.

if they are the same, then no problem to merge them...

Copy link
Member

Choose a reason for hiding this comment

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

i think that there is one concept here and that we would be well served by keeping one concept; i think the subtle distinction may be too hard to explain.

I think i prefer then input name baseuri to uri so I'd advocate using this throughout and having only 1 input parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion makes sense Mark. Let's go with that. I've also added a test for RDF graph output with a baseuri so that we're covered for that.

file_variables = {}
for name in fhandle.variables:
#print(name)
if name == prefix_var_name or name == alias_var_name:
Copy link
Member

Choose a reason for hiding this comment

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

this appears to be doing the same as skipped_variables (removed at old l591) but skipped_variables still exists. should it be removed altogether in favour of this pattern, or kept instead of this pattern.

I'd prefer to decide one way or the other and clean up unused code if we can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems like we had developed the same functionality concurrently, but the issue I had was that i had commits which depended on this pattern. rolling it back and figuring out the differences was a bit of a challenge, which is why i ended up this way.

@jyucsiro
Copy link
Contributor Author

jyucsiro commented Oct 2, 2017

Yes, this PR encapsulates #53 and #56 - mostly issues you've identified in the elements above.

@marqh
Copy link
Member

marqh commented Oct 4, 2017

i think just the uri|baseuri to deal with, then this is good to merge

thank you @jyucsiro

@marqh marqh merged commit b25b2ec into binary-array-ld:master Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants