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

feat!: create Ratio and use it for aspectRatio and displayAspectRatio and frameRate #336

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

MarcAntoine-Arnaud
Copy link
Contributor

issue #332

@MarcAntoine-Arnaud MarcAntoine-Arnaud changed the title feat!: create Ratio and use it for aspectRatio and displayAspectRatio feat!: create Ratio and use it for aspectRatio and displayAspectRatio and frameRate Jun 19, 2024
@JuergenGrupp
Copy link
Collaborator

Thank you @MarcAntoine-Arnaud !
If the pull request is approved, it needs to be merged to the "dev" branch, not the "main".
Please see : https://github.com/ebu/ebucoreplus/wiki/04-Branching-policy-for-main,-dev-and-issue%E2%80%90branches

@MarcAntoine-Arnaud MarcAntoine-Arnaud changed the base branch from main to dev June 20, 2024 16:53
owl:onDataRange xsd:integer ;
xsd:minInclusive 1
] ;
dcterms:description "Represent a ratio of values."@en ,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo : Represents

owl:minQualifiedCardinality "1"^^xsd:nonNegativeInteger ;
owl:maxQualifiedCardinality "1"^^xsd:nonNegativeInteger ;
owl:onDataRange xsd:integer ;
xsd:minInclusive 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about the restriction : xsd:minInclusive 1
Don't you think you should encapsulate this restriction within a datatype definition ? To be OWL 2 compliant ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long review time! During our last Editorial Committee meeting we have concluded to pack this into a new issue and use a new class Ratio for two of the three attributes mentioned above. I have now re-analysed the comments and would also use it for the Attribute frameRate.
But as I am starting to do this, I am stumbling across this question of aro-max. What do you think, @MarcAntoine-Arnaud ? Should you encapsulate the restriction within a datatype definitrion as proposed by @aro-max ? If we can resolve this question quickly, I might be able to review, merge and commit your pull request quickly as well.

@JuergenGrupp
Copy link
Collaborator

JuergenGrupp commented Jul 8, 2024

Currently the property displayAspectRatio has the range "ActiveFormatDescriptorCode" (which is a subclass of "Format"). It is probably intended to use the classification scheme "ebu_ActiveFormatDescriptorCodeCS" here, as defined in http://www.ebu.ch/metadata/cs/EBU_cs_p.zip.
Please note, this Format Descriptor Code expresses more than only a ratio. I fear we might lose important information if we reduce it to the class "Ratio".
I am not sure either about how to handle the property aspectRatio: should we better extend this to also use the "ActiveFormatDescriptorCode", should we change the range to the "Ratio" class or should we leave it as it is?

@JuergenGrupp
Copy link
Collaborator

Editorial Committee:

  • we need both properties, one represents the recording format and the other representing the display format, because in rare cases they will be different
  • but both properties should take the numerator/denominator style value.

Conclusion: we will create a separate issue on that

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