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 Artefact Type Certainty node #1281

Open
wants to merge 4 commits into
base: dev/1.0.x
Choose a base branch
from

Conversation

SDScandrettKint
Copy link
Member

@SDScandrettKint SDScandrettKint commented Jul 22, 2024

Addresses #1273

Adds Artefact Type Certainty and Artefact Type Certainty Metatype nodes on the same level as Artefact Type.
image
image

Resulting card looks as follows:
image

@Philbox Need I'm sunsure whether Phase Classification is the correct parent nodegroup for Artefact Type Certainty, or if you think Artefact Type would be a better parent for it?
image

@SDScandrettKint SDScandrettKint self-assigned this Jul 22, 2024
@SDScandrettKint SDScandrettKint requested review from csmith-he and Philbox and removed request for csmith-he July 22, 2024 13:34
@SDScandrettKint SDScandrettKint added the modelling dealing with the semantics of a model or branch label Jul 22, 2024
@csmith-he
Copy link
Contributor

@SDScandrettKint Working as expected in the UI. Just to check, this Artefact Type Certainty isn't expected to be displayed in the reports, is it?

@SDScandrettKint
Copy link
Member Author

SDScandrettKint commented Jul 23, 2024

@SDScandrettKint Working as expected in the UI. Just to check, this Artefact Type Certainty isn't expected to be displayed in the reports, is it?

Thanks for spotting this @csmith-he, I forgot about adding it to the report! It has been added in the latest commit (855507a). I don't think it's necessary for it to have its own column, so I have added it in the "show details" part.

@csmith-he
Copy link
Contributor

Just to confirm from a functionality standpoint, I've managed to load our test cut of GLHER data into an instance of AfHERs with these changes in the models without any ill effects that I can see.

From this point on, it just needs to be looked at by @Philbox now

@Philbox
Copy link
Collaborator

Philbox commented Aug 20, 2024

@SDScandrettKint Hang this off artefact type rather than production. Otherwise it's fine.

@aj-he
Copy link
Collaborator

aj-he commented Aug 28, 2024

@SDScandrettKint - Once you have changed the parent node of the new Artefact Type Certainty node as suggested by @Philbox, we can get this merged.

@SDScandrettKint
Copy link
Member Author

As of 681a0f2, Artefact Type Certainty (+ metatype) have been moved under Artefact Type:
image

Both Artefact Type Certainty and Artefact Type Certainty Metatype have the Ontology Class E55_Type and the relation to parent (Artefact Type) P2_has_type. Previously, Artefact Type Certainty had the relation of P141_assigned, but it is no longer available in the dropdown and hardcoding it in the model JSON causes errors
image

@aj-he
Copy link
Collaborator

aj-he commented Aug 28, 2024

@SDScandrettKint - I've asked @Philbox to take a look

@Philbox
Copy link
Collaborator

Philbox commented Aug 28, 2024

@SDScandrettKint the P141 assigned property is no longer true because that relates to the classification assignment. P2 has type is the correct property.

So E55 Artefact Type -> P2 has type -> E55 Artefact Certainty Type

This way the certainty type has a semantic relationship with Artefact Type and not any other node in the Phase Classification.

@aj-he
Copy link
Collaborator

aj-he commented Aug 29, 2024

Review on hold until graph changes discussed and agreed by group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modelling dealing with the semantics of a model or branch
Projects
Status: Requires graph change approval
Development

Successfully merging this pull request may close these issues.

4 participants