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

Add in Osm and mOsm/kg for measurement done with Osmometer #1085

Merged
merged 10 commits into from
Jan 10, 2025

Conversation

Toby-Broom
Copy link
Contributor

I added back the Osm units for review

Copy link
Collaborator

@steveraysteveray steveraysteveray left a 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.

@steveraysteveray
Copy link
Collaborator

Oh, and also, don't forget to update CHANGELOG.md!

@Toby-Broom
Copy link
Contributor Author

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.

The other osmotic ones use this sort of description so should probably refactor these as well?

Copy link
Collaborator

@steveraysteveray steveraysteveray left a 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
@Toby-Broom
Copy link
Contributor Author

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.

Copy link
Collaborator

@steveraysteveray steveraysteveray left a 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.

src/main/rdf/vocab/unit/VOCAB_QUDT-UNITS-ALL.ttl Outdated Show resolved Hide resolved
@steveraysteveray
Copy link
Collaborator

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.

@Toby-Broom
Copy link
Contributor Author

Toby-Broom commented Jan 8, 2025

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

@fkleedorfer
Copy link
Collaborator

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.

@steveraysteveray
Copy link
Collaborator

@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?

@steveraysteveray
Copy link
Collaborator

@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.

@steveraysteveray
Copy link
Collaborator

Same for unit:MilliOSM-PER-KiloGM

@fkleedorfer
Copy link
Collaborator

@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.

@fkleedorfer fkleedorfer merged commit e94a87e into qudt:main Jan 10, 2025
1 check passed
@Toby-Broom Toby-Broom deleted the tb-osmol branch January 10, 2025 18:05
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.

3 participants