-
-
Notifications
You must be signed in to change notification settings - Fork 84
Fix several errors found with ABECTO #526
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
Conversation
We looked at some of the discrepancies you noted and will review others.
The ABECTO comparison seems to be a valuable tool, but we do not think it
should be run on every pull or pr request. Rather, we would want to run
this comparison on demand and would appreciate you modifying the .yml file
accordingly, as well as to provide documentation on how to perform the
comparison on demand if it isn't obvious. We will place the comparison
results into a folder that isn't included in the release.
Jack Hodges, QUDT
…On Tue, Jul 19, 2022 at 9:08 AM Jan Martin Keil ***@***.***> wrote:
@jmkeil <https://github.com/jmkeil> requested your review on: #526
<#526> Fix several errors
found with ABECTO as a code owner.
—
Reply to this email directly, view it on GitHub
<#526 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATQRWLPIOGEYJZNNCZ3VRTVU3HG7ANCNFSM54ARD7CA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
Jack
|
@jmkeil, thanks for the listing. I'm afraid we cannot just adopt the dimension vector changes you have in this PR, because our Units, QuantityKinds and DimensionVectors links involve more than just a given change for a QuantityKind. For example, for quantitykind:BodyMassIndex, you correctly point out that the length vector should be -2, not -1. However, in addition to that change, two Units point to quantitykind:BodyMassIndex (unit:KiloGM-PER-M and unit:KiloGM-PER-MilliM). They should no longer point to quantitykind:BodyMassIndex, so those triples should also be removed and replaced with references to quantitykind:MassPerArea (and others of like dimension as appropriate). This also impacts our automatic generation of applicableUnits, as described here. Finally, our QuantityKinds often are part of a skos:broader hierarchy where the broader and narrower entries all have the same dimensionality. That happens to not be the case with quantitykind:BodyMassIndex, but in general we need to move the quantity kind out of one skos:broader hierarchy and into another with the correct dimensionality. We plan on correcting the errors you point out, but as I think you'll agree, this is more involved than one might think! We will proceed carefully. |
@steveraysteveray thanks for reviewing the changes and the feedback. I wasn't aware of these hierarchy, but of course that needs to get adjusted, too. I converted the PR into a draft and you might use it as base for the further needed adjustments. |
Thanks! |
I just noticed, that I forgot to grant you edit privileges on this branch, but have made up for it now. Sorry. Was it intended to close the PR? |
Yes, I decided that rather than cherry-pick my way through your fixes, it was easier to close this PR, but work on a subset of the discrepancies in a new branch, which was merged on 2022-07-27. I started by addressing the dimension vectors because I felt that was structurally critical to get right. Each quantity kind noted in your spreadsheet takes research and cross-checking with other quantity kinds via skos:broader and with the units referring to it, so it's kind of slow. Of the 15 quantity kinds identified, I have addressed 10 of them in the latest merge. I'm curious how your tool identifies the corresponding entities in the other ontologies, because, for example, what is known as "Magnetic Field" can be expressed in two different ways, related by Maxwell's equations. Our http://qudt.org/vocab/quantitykind/MagneticField describes the field in terms of B, and the OM ontology describes it in terms of H. They are not directly comparable. The OM version corresponds to what we call the AuxillaryMagneticField. However, now that I have edit privileges on the branch, maybe I should go back and take advantage of your other changes as well, so I'm re-opening. Thanks. |
The basic mapping is done based on String similarity of the labels (see c904f9b#diff-e48e569c0c233b06e9a61b27cf0f6ddedcd8142da4c91bf3d2f8e0debbbbc6b9R789) and is complemented by manual corrections in the pipeline configuration (see e.g. 6a06b8a#diff-e48e569c0c233b06e9a61b27cf0f6ddedcd8142da4c91bf3d2f8e0debbbbc6b9R748). |
I have resolved the conflicts and incorporated your suggested changes that we agree with so far, and created a new branch and PR: "abecto errors". So I'm closing this one, having noted the remaining outstanding things to consider. |
Hi. This fixes several errors found with the comparison using ABECTO (#525).