-
Notifications
You must be signed in to change notification settings - Fork 20
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
complexValue required type fixed #89
Conversation
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 "required" should by on the "typedQuantities" property.
"properties": {
"typedQuantities": {
"description": "List of quantities required to fully describe the complex value",
"items": {
"$ref": "#/definitions/TypedQuantity"
},
"type": "array"
},
"required": [ "typedQuantities" ]
},
the removed part was ok:
"required": [
"quantityType",
"quantity"
]
Thank you @redmitry to spot that! I added another commit to fix the position of the required. Can you review it again and confirm it's correct now? Thanks. |
The last piece is to return back: "required": [
"quantityType",
"quantity"
] 😄 |
ok, done @redmitry , I am starting to feel the boomerang effect with this PR jaja, can you review it again? thank you! |
It looks correct now. "measurementValue" : {
"quantity" : {
"unit" : {
"id" : "NCIT:C49671",
"label" : "Kilogram per Square Meter"
},
"value" : 26.63838307
}
} while correct one must be like: "measurementValue": {
"unit": {
"id": "NCIT:C49671",
"label": "Kilogram per Square Meter"
},
"value": 26.63838307
} I have a fixed version: |
Just a note, if we want to be Phenopacket friendly, I suggest changing our schema to align with theirs. When in trouble, I follow theirs... See: https://phenopacket-schema.readthedocs.io/en/latest/value.html and: EGA-archive/beacon2-ri-tools#3 Cheers, m |
Ok, let's close this PR and discuss the individuals schema question in issues. @mbaudis can you please review and accept this PR? Thanks. |
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.
Changes are good but there is an example (both yaml & json) which says "quentity"... I wonder who the original committer was ...
Tru dat. But PXF is inconsistent itself, at least when looking at the examples (and hampered by the "let's write this in YAML starting from a protobuf definition" documentation...):
But example in Measurement:
... places the |
we need @redmitry approval here to close this PR |
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.
remember to validate individuals over fixed schema.
yes @redmitry , I still have to take a look carefully at individuals issue before validating it. |
@redmitry I just checked the CINETICA individuals schema. Correct me if I'm wrong but I think the correction you made is because the value is not complex? Also, even if the value was complex, quantity should be an array not an object? |
Beacon's schema != phenopackets. We may create an entry type "phenopackets" and provide a schema for it.
Cheers, D. |
In my view, the difference between Beacon v2 and Phenopackets v2 schemas, specifically in the The ideal scenario would be Phenopackets folks serializing their schema to JSON (if anybody from Phenopackets is reading this, please listen to us!! 😄). However, if this isn't feasible, I propose forming a small working group of two/three people (@mbaudis) to identify and gradually address these differences. I would be eager to participate, given the impact on my work. Cheers, m |
Ok, then at the moment we will stick to specifications and give the CINECA dataset the schema suggested by @redmitry for measurementValues until we find a solution for the adaptability to Phenopackets. |
@costero-e, IMO the key consideration here is whether the exclusion of the hierarchy 'quantity' from the Phenopacket schema was intentional or simply due to the challenges associated with reverse engineering the Protobuf schema. It should be noted that as of 2021, there were not many Phenopacket schemas available for 'Value'. https://phenopacket-schema.readthedocs.io/en/master/value.html If the downstream of the object 'Value' in Beacon v2 is identical to that in Phenopackets, it would be more reasonable to maintain consistency upstream as well. Instead of attempting to modify all replicas of CINECA's 'individuals.json', which is likely one of the few, if not the only, real examples that people have of Beacon v2 Models 😅 , it would be preferable to align the schema upstream and move on. What do you think @mbaudis? Cheers, Manu |
The problem is that "phenopackets-schema" is not a JSON schema. The question is if it is feasible to develop "phenopackets-like" json schema, because in this case phenopackets would need to provide serializer/deserializer for it. The choices are:
Cheers, Dmitry |
Hi @mrueda and @redmitry. I agree with both of you and as we need to find a solution for CINECA individuals schema respecting both specifications and future phenopackets interoperability I would propose two solutions (if I am wrong or is not feasible what I propose, correct me, please):
|
This is the fix for the issue opened by @redmitry here #87 . Please @mbaudis and @redmitry review it to move this forward. Thank you.