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

SHACL work for Brick #97

Closed
wants to merge 102 commits into from
Closed

Conversation

xyzisinus
Copy link
Contributor

This PR is a continuation of PR 85. It encompasses the following items:

  1. A script generating shapes using Brick properties and its accompany test cases.
  2. A thin wrapper of pyshacl finding offending triples reported as constraint violations.

Special notes:

  1. rdfs:domain and rdfs:range are temporarily replaced with brick:expectedDomain and brick:expectedRange as a workaround for lacking of disjointness enforcement. However I choose not to modify the Brick.ttl file at the root of the repo. Instead, a Brick.ttl is generated with above said properties and used only as the ontology file for pyshacl.
  2. The substance_subproperties in Gabe's generate_shacl.py has been moved into properties.py as a sub-property of brick:feeds, expressed as brick:hasSubproperty. This is a modification to Brick Schema -- needing special attention by the reviewer.

gtfierro and others added 30 commits October 22, 2019 13:39
@xyzisinus
Copy link
Contributor Author

xyzisinus commented Jan 30, 2020

 OWL.inverseOf: "hasLocation"

I'm puzzled -- I don't have that line in the file.

But I notice that in the master of Brick repo, the notation for the brick terms has changed from BRICK.hasLocation to BRICK["hasLocation"]. I thought that's some global decicion and I'm going through the file to convert things like BRICK.expectedDomain into BRICK["expectedDomain"].

@gtfierro
Copy link
Member

BRICK.hasLocation and BRICK["hasLocation"] are equivalent. I prefer the latter, but it shouldn't make a difference from a correctness standpoint. I would still check that the value-side is a valid Literal or URIRef, as that is what is suggested by the build errors

@xyzisinus
Copy link
Contributor Author

xyzisinus commented Jan 30, 2020

BRICK.hasLocation and BRICK["hasLocation"] are equivalent. I prefer the latter, but it shouldn't make a difference from a correctness standpoint. I would still check that the value-side is a valid Literal or URIRef, as that is what is suggested by the build errors

       "subproperties": {
            "feedsAir": {
                SKOS.definition: Literal("Passes air"),
                'substance': BRICK["Air"],    <<<< error 
                'properties': [BRICK.regulates, BRICK.measures]
            },
        },

This is the offending part. "substance" is not a URI.

@gtfierro
Copy link
Member

Oh, bizarre. Why is the 'substance' key in there? That's not an accepted key in the Brick generation process

@xyzisinus
Copy link
Contributor Author

Oh, bizarre. Why is the 'substance' key in there? That's not an accepted key in the Brick generation process

I added this part. It's was hard coded in generate_shacl.py. I moved it here. Is that the right thing to do?

@gtfierro
Copy link
Member

The files used by generate_brick.py have a certain structure that, while we can expand, does not accept keys/values that don't adhere to the expected types. Leave the "substance" annotation in generate_shacl.py for now (or eliminate it all together if you don't need it). After you remove it from generate_brick.py, the tests should start passing

Copy link
Member

@gtfierro gtfierro left a comment

Choose a reason for hiding this comment

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

Nice work!

I think what's next is making the SHACL shapes as complete as possible.

  • SHACL shape and unit test for each Brick property
  • examples of SHACL shapes (with lower severity) for an "equipment template". What I mean by this is a shape that encapsulates the expected points (and other properties) for a given type of equipment. For example, a VAV shape for a particular building might require that the VAV has a Damper (through hasPart), a temperature sensor and setpoint (through hasPoint), an upstream AHU (isFedBy) and a downstream HVAC Zone (feeds). These won't be "universal" in that we'll use them for all Brick models, but we should show how one would create such a shape for a particular deployment

bricksrc/properties.py Outdated Show resolved Hide resolved
bricksrc/properties.py Outdated Show resolved Hide resolved
shacl/DESIGN.md Outdated Show resolved Hide resolved
shacl/generate_shacl.py Show resolved Hide resolved
Also new use of rdfs:domain and :range crept in from the master.
Change them to brick:expectedDomain and :expecteRange.
@xyzisinus
Copy link
Contributor Author

xyzisinus commented Feb 9, 2020

  • SHACL shape and unit test for each Brick property
  • examples of SHACL shapes (with lower severity) for an "equipment template". What I mean by this is a shape that encapsulates the expected points (and other properties) for a given type of equipment. For example, a VAV shape for a particular building might require that the VAV has a Damper (through hasPart), a temperature sensor and setpoint (through hasPoint), an upstream AHU (isFedBy) and a downstream HVAC Zone (feeds). These won't be "universal" in that we'll use them for all Brick models, but we should show how one would create such a shape for a particular deployment

Also it will be good to draw insights via sparql from Brick.ttl. For example when I make shapes for feedsAir as it is, I have to specify substanceSuperClass. But this info can be obtained by querying for classes that have Brick:Air as a subclass.

I'll put those thoughts into the future plan in DESIGN.md.

@xyzisinus
Copy link
Contributor Author

I have send thought on expectedDomain/Range stuff in properties.py. They should not be there because the normal Brick.ttl is generated using the file, too. I'd put .domain and .range back. Then use a simple tool to remove .domain and .range from standard Brick.ttl. Use this modified Brick.ttl for shacl reasoning.

@xyzisinus
Copy link
Contributor Author

Put Gabe's email here for the records:

  1. SHACL definitions for Brick: We need a set of restrictions (called "shapes" in SHACL-speak) that verify that the Brick relationships are being used appropriately. E.g. if the "brick:feeds" relationship appears in a Brick model, then the subject and object of that relationships should have compatible types. Charlene has at least a few of these working; I need to verify the completeness but I think we're almost there

  2. Integrate SHACL into the tooling: how do people use the SHACL shapes we are defining? I know Charlene has a validate.sh script but it would be nice if some of this functionality was folded into the https://github.com/BrickSchema/py-brickschema package so that it was only a couple lines of Python away from being used. Maybe in a validation.py submodule? We also need to put documentation for the SHACL validation procedure into the main README of the Brick repository

  3. Examples of SHACL shapes for equipment templates: a use case that has been brought up several times by potential users of Brick is how to ensure that certain modeling idioms are followed. These exist at a site- or model- level, rather than at the level of the ontology like the shapes I mentioned above. For a particular site, we may know that all of our VAVs should have a brick:isFedBy relationship to an AHU; we can "enforce" this idiom on the model through the use of a SHACL shape (all VAVs should have an isFedBy relationship to an instance of the AHU class). We can't place this restriction on all uses of Brick, but we should make it possible to define a shape that enforces this restriction for a particular model. Eventually, it would be nice to have a library of these. Developing the library is a subject of future work. The concrete task I'd like Charlene to handle is implementing and documenting the process of including such optional shapes into the validation process. This part should probably be implemented in the https://github.com/BrickSchema/py-brickschema package

@xyzisinus
Copy link
Contributor Author

Before going ahead with major changes let us see if the following moves sound reasonable.

  1. First I propose to remove expectedDomain, expectedRange from properties.py. Since Brick.ttl is generated using the file, changing the file means changing the Brick schema -- not a light design decision. Instead, I propose that we feed pyShacl a modified Brick.ttl (with rdfs:domain and :range reamoved) for pyshacl reasoning. This approach has been coded and tested. The remaining work is to move the Brick.ttl modification code from generate_shacl.py into the shacl submodule in py-brickschema. DESIGN.md should be changed accordingly -- maybe moved to py-brickschema, too?
  2. Build a submodule shacl (name?) in py-brickschema. The module should have the following api: a validate() function very much like pyShacl's but it takes Brick.ttl and the output of generate_shacl.py by default; a way to add a user-defined shape as a graph (.addShape()) and a way to add a user-defined shape file (.importShapes()). Outpuf of validate(), the results_graph is similar to pyShacl's but with extra triples predicated with bsh:offendingTriple to tell user what triples are incorrect.
  3. Code samples in the shacl module's document to show how to use the package and explain the expected outcome.
  4. Unit tests in py-brickshema.
  5. Remaining issues: I assume generate_shacl.py will remain in the Brick repo. Then how does the shacl module in py-brickschema get the generated shape file? Should the file be available from our Brick website like Brick.ttl?

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