-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add in Osm and mOsm/kg for measurement done with Osmometer #1085
Conversation
… measurements of ionic solutions
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.
In the description of the OsmoticConcentration, I'm a little uncomfortable with using the word "quantity" as it might cause confusion related to qudt:Quantity. Would you consider some alternative natural language word, or just use QuantityKind?
Secondly, we need to define unit:OSM if we are to define MilliOSM-PER-KiloGM.
Oh, and also, don't forget to update CHANGELOG.md! |
The other osmotic ones use this sort of description so should probably refactor these as well? |
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.
The quantitykind:OsmoticConcentration description and dimension vector tell me it is moles per volume. But the unit:MilliOSM-PER-KiloGM shows a dimension of moles per (volume x mass), so it cannot have qudt:hasQuantityKind of OsmoticConcentration since they are of different dimensionalities. Could you adjust and create additional quantity kinds if needed?
# Conflicts: # src/main/rdf/vocab/quantitykinds/VOCAB_QUDT-QUANTITY-KINDS-ALL.ttl # src/main/rdf/vocab/unit/VOCAB_QUDT-UNITS-ALL.ttl
I think I made a mistake, I did osmotic concentration over mass but it should be osmol/mass so it's then the same as qk: AmountOfSubstancePerMass. I defined the unit osmole which is same as mole but specific to the osmotically active entity. I think normally people would use the osm/L variant like like mol/L but in this case they are using the mass based version. |
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.
Two changes requested. See embedded comments.
So sorry for dragging this out. Juggling multiple changes. Your latest fixes look great. There's now the merge conflict because of the maven update we just made. Would you mind resyncing your branch with our main, and resolving the conflict in changelog.md? Just need to add your lines as well as to the other new lines in changelog.md. Again, sorry for the delay. |
No worries, mostly me :) Also, we make our own custom individuals so we can solve them in the short term but I like to push things upto you so everyone else can benift from the standardisation you bring |
# Conflicts: # CHANGELOG.md
Some SHACL checks failed, please look at the output of the CI Build. In that log file there are some errors reported for the newly added entities. E.g., the conversionMultiplier must be 0.0, not 0. Please let us know if you need further assistance. Some of the messages there do not pertain to the triples that were added. Most likely they are only warnings. |
@fkleedorfer, since some of the errors derive from the datatypes graph, can we exclude that graph from the validation temporarily until @ralphtq has had a chance to make those fixes? |
@Toby-Broom, I found a subtle error in your submission for unit:OSM. The dimension vector contains "TD0" where it should contain "T0D0", and thus does not find it. |
Same for unit:MilliOSM-PER-KiloGM |
@steveraysteveray removing the datatypes graph from validation does not make the validation results (severity: info) go away - the entities that cause them are also in the schemas, apparently, which, I believe, we do need. The only problem with the validation results is that their severities are not printed in the maven output. When I find the time to update the shacl-maven-plugin, we can do something about that - but there isn't really anything we can do in our project right now. |
I added back the Osm units for review