-
Notifications
You must be signed in to change notification settings - Fork 1
BatteryPass Aspect Models of joint Catena-X/IDTA expert modelling group - Version for Review #10
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
base: main
Are you sure you want to change the base?
Conversation
Bi bo/published cfp
Bi bo/published cfp
fix pcf see attribute values for IRDI
…ell-io/smt-semantic-models into BiBo/batteryPass_models
operatorId optional for Battery Nameplate
|
If you have any findings you want to submit via PR please provide them to the branch https://github.com/admin-shell-io/smt-semantic-models/tree/smt-xg/BatteryPass_Working |
tobzahn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General questions:
- where are the formats and units specified? E.g. there are some temperatures - where is it specified what unit (Celcius, Fahrenheit) is to be used? Or for date attributes, what data format (e.g. YYYY-MM-DD or YYYY-DD-MM) to be used?
- Is it correct that dynamic data (to be updated while the battery is in operation) is only to be found in batterypass.product_condition? Therefore, all other data models are to be filled out by the manufacturer and remain unchanged (static)?
- Will there be a textual document (e.g. a Catena-X Standard) that explains how to use the data model, how to make it available to customers and how the exchange is supposed to work?
io.admin-shell.idta.carbon_footprint/1.0.0/ProductOrSectorSpecificCarbonFootprints_generic.ttl
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint/1.0.0/ProductOrSectorSpecificCarbonFootprints_shared.ttl
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint.pact/1.0.0/gen/CarbonFootprintPact.json
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint.pact/1.0.0/gen/CarbonFootprintPact.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.circularity/1.0.0/gen/CircularityBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.digital_nameplate/1.0.0/gen/NameplateBattery.json
Show resolved
Hide resolved
|
reopened because this PR was mentioned to be the one to add changes in the call for review in Catena-X. |
In Aspect Models unit handling see https://eclipse-esmf.github.io/samm-specification/snapshot/units.html and https://eclipse-esmf.github.io/samm-specification/snapshot/appendix/unitcatalog.html If units are missing then a corresponding issue shall be raised. If it is missing in referenced reused model the issue shall be raised in the corresponding github of the reused model.
yes, this is the intention. If you find any dynamic data in other models this should be considered a bug
sure,
|
io.admin-shell.idta.batterypass.technical_data/1.0.0/gen/TechnicalDataBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.technical_data/1.0.0/gen/TechnicalDataBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.technical_data/1.0.0/gen/TechnicalDataBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint.pact/1.0.0/gen/CarbonFootprintPact.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.circularity/1.0.0/gen/CircularityBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.circularity/1.0.0/gen/CircularityBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.circularity/1.0.0/gen/CircularityBattery.json
Show resolved
Hide resolved
io.admin-shell.idta.handover_documentation/2.0.0/HandoverDocumentation.ttl
Show resolved
Hide resolved
io.admin-shell.idta.handover_documentation/2.0.0/HandoverDocumentation.ttl
Show resolved
Hide resolved
io.admin-shell.idta.handover_documentation/2.0.0/HandoverDocumentation.ttl
Show resolved
Hide resolved
|
Additional comment addressed here: eclipse-tractusx/sldt-semantic-models#876 (comment)
|
|
General comments:
|
sebastiankb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #10 (comment)
io.admin-shell.idta.batterypass.digital_nameplate/1.0.0/BatteryNameplate.ttl
Show resolved
Hide resolved
| samm:description "Contains the nameplate information attached to the product within the Battery Product Passport."@en ; | ||
| samm:description "Enthält die Information zum Typenschild des Produkts im Digitalen Batteriepass."@de ; | ||
| samm:properties ( | ||
| [ samm:property nameplate:uriOfTheProduct; samm:payloadName "URIOfTheProduct" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this causes wrong semantic IDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no roundtrip, you are right but the semanticIds are added via see-attribute
| samm:preferredName "URI of the product"@en ; | ||
| samm:description "Unique global identification of the product using an universal resource identifier (URI)."@en ; | ||
| samm:description "Global eindeutige Identifikation des Produkts."@de ; | ||
| samm:see <urn:irdi:0112/2///61987%23ABN590%23002> ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is should be the semantic ID of the uriOfTheProduct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IRDI Are not supported as value for see-attribute in SAMM, therefore urn:irdi: added as prefix. The IRDI itself should be correct, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if they not show-up in the AAS version, then this is ok
io.admin-shell.idta.batterypass.digital_nameplate/1.0.0/BatteryNameplate.ttl
Show resolved
Hide resolved
|
Please also see my comments at PR #34 |
This is the branch for incorporating the findings of the review.The original branch for the review can be see here: #33