Azure / opendigitaltwins-building

Open Digital Twins Definition Language (DTDL) RealEstateCore Ontology
MIT License
150 stars 42 forks source link

Creating Elevator Trip Event for Elevator Conveyance #61

Closed akshayj-MSFT closed 2 years ago

hammar commented 2 years ago

Hi,

Nice addition! One question, that I missed asking before you merged: why use properties with string schemas for level identifiers, when we have levels as interfaces in the model? Might not relationships to those Level twins be a better model?

Karl

rszcodronski commented 2 years ago

I can try to answer that one since it was my proposal...

An Elevator trip event is an event which occurs very often every day/minute/second and we've modeled its "parent" (aka producer) to be the elevator cab itself. Because those level identifiers are constantly changing as an elevator goes up and down, I assumed we wouldn't want to be constantly creating/deleting relationships with the Level twins. Instead, we can just trend those ids similar to how we trend the lastValue for the Capabilities.

Is my assumption around not wanting to be rapidly creating and deleting those relationships correct?

hammar commented 2 years ago

I can try to answer that one since it was my proposal...

An Elevator trip event is an event which occurs very often every day/minute/second and we've modeled its "parent" (aka producer) to be the elevator cab itself. Because those level identifiers are constantly changing as an elevator goes up and down, I assumed we wouldn't want to be constantly creating/deleting relationships with the Level twins. Instead, we can just trend those ids similar to how we trend the lastValue for the Capabilities.

Is my assumption around not wanting to be rapidly creating and deleting those relationships correct?

From a pure data modelling point-of-view, I'd argue that every time someone rides the elevator or presses the elevator button, it is a separate event, that runs between two places., and where the state is updated (e.g., current level relationships). From that point-of-view you wouldn't create and remove relationships all the time, you'd rather create twins. But I recognize that the overhead of such a solution might make it less palatable to devs.

However, if we are instead meaning to represent the current state of the building at any given point in time, then arguably the position, current direction, and target of the elevator is an attribute of the elevator itself, not some separate event instance?

I'm not going to fight on this, I can live with the design, it just doesn't look "just right" for me :-)

hammar commented 2 years ago

PS: I'll include in the OWL models and re-generate DTDL from this shortly. It'll be the same representation, possibly with some shift in the order of properties.

hammar commented 2 years ago

PS: I'll include in the OWL models and re-generate DTDL from this shortly. It'll be the same representation, possibly with some shift in the order of properties.

When doing this I noticed two things:

  1. There is no linkage between this new event and the Asset/Elevator interface.
  2. If such a linkage were added, perhaps the Event itself should also be placed under the Asset namespace, rather than core.

@akshayj-MSFT and @rszcodronski, are you OK if I fix the above two matters in a new PR? If yes, should the relationship direction be Elevator -> TripEvent or TripEvent -> Elevator?

rszcodronski commented 2 years ago

Good catch.

In our Willow ontology where we have added this, I also added a relationship on the top-level Event interface called producedBy. My thinking is that this is common terminology in our event-driven world where there are producers and consumers. This relationship doesn't have a target so an "Event" could be attached to any parent interface as its "producer". In this example, the ElevatorTripEvent-->producedBy-->Elevator. In another example, we have PersonAccessEvent-->producedBy-->Person.

Because events could be produced by any number of our top-level interfaces, I like keeping it with its own namespace outside of Asset. Conceptually, I'm thinking about Events as a sister-class to Capabilties. Where Capabilities can be attached to any top-level interface and have a simple data model (lastValue), the Events have a more complex data model (i.e. startLevel, endLevel, currentLevel).

Thoughts?

{
  "@type": "Relationship",
  "description": {
    "en": "The entity responsible for generating or producing the event."
  },
  "displayName": {
    "en": "Produced By"
  },
  "name": "producedBy"
},
akshayj-MSFT commented 2 years ago

this seems good to me

On Fri, Sep 24, 2021 at 1:51 AM Karl Hammar @.***> wrote:

PS: I'll include in the OWL models and re-generate DTDL from this shortly. It'll be the same representation, possibly with some shift in the order of properties.

When doing this I noticed two things:

  1. There is no linkage between this new event and the Asset/Elevator interface.
  2. If such a linkage were added, perhaps the Event itself should also be placed under the Asset namespace, rather than core.

@akshayj-MSFT https://github.com/akshayj-MSFT and @rszcodronski https://github.com/rszcodronski, are you OK if I fix the above two matters in a new PR? If yes, should the relationship direction be Elevator -> TripEvent or TripEvent -> Elevator?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Azure/opendigitaltwins-building/pull/61#issuecomment-926459589, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATBLNFAPWHLVJ5V77SRK7DUDQ3Y3ANCNFSM5ENB3MHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.