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 MonetaryPerDuration and CurrencyPerDurationUnit classes. Fixes #846 #847

Closed
wants to merge 2 commits into from

Conversation

rjyounes
Copy link
Collaborator

@rjyounes rjyounes commented May 17, 2023

Fixes #846.

…dition to release note documentation in Contributing.md.
@rjyounes rjyounes changed the title Add MonetaryPerDuration and CurrencyPerDurationUnit classes. Fixes #817 Add MonetaryPerDuration and CurrencyPerDurationUnit classes. Fixes #846 May 17, 2023
@rjyounes rjyounes requested review from uscholdm and dylan-sa May 17, 2023 13:06
Copy link
Contributor

@dylan-sa dylan-sa left a comment

Choose a reason for hiding this comment

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

One thought on the CurrencyPerDurationUnit formal def, but otherwise looking good to me.

a owl:Class ;
owl:equivalentClass [
a owl:Class ;
owl:intersectionOf (
Copy link
Contributor

@dylan-sa dylan-sa May 17, 2023

Choose a reason for hiding this comment

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

Can ratio units have numerators or denominators w/ more than one unit of measure? If not, is this a case where an allvaluesFrom + cardinality = 1 construction would better capture the meaning? (Maybe the cardinality restrictions would be better placed in the RatioUnit definition; and if so, maybe this should be raised as a separate issue.)

allValuesFrom + cardinality = 1 version:

gist:CurrencyPerDurationUnit
	a owl:Class ;
	owl:equivalentClass [
		a owl:Class ;
		owl:intersectionOf (
			gist:RatioUnit
			[
				a owl:Restriction ;
				owl:onProperty gist:hasDenominator ;
				owl:allValuesFrom gist:DurationUnit ;
			]
			[
				a owl:Restriction ;
				owl:onProperty gist:hasNumerator ;
				owl:allValuesFrom gist:CurrencyUnit ;
			]
			[
				a owl:Restriction ;
				owl:onProperty gist:hasNumerator ;
				owl:cardinality 1 ;
			]
			[
				a owl:Restriction ;
				owl:onProperty gist:hasDenominator ;
				owl:cardinality 1 ;
			]
		) ;
	] ;

Copy link
Contributor

@uscholdm uscholdm May 18, 2023

Choose a reason for hiding this comment

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

@dylan-sa @rjyounes

Can ratio units have numerators or denominators w/ more than one unit of measure?

Not sure what you mean. For example, if the denominator is a duration unit of one hour, then as stated you are asking if the unit of one hour itself has more than one unit of measure. But a unit of measure doesn't generally have even one unit of measure, it IS a unit of measure. So maybe you are asking whether a ratio unit can have more than one denominator (or numerator). I think the answer should be no.

Copy link
Contributor

@dylan-sa dylan-sa May 18, 2023

Choose a reason for hiding this comment

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

So maybe you are asking whether a ratio unit can have more than one denominator (or numerator). I think the answer should be no.

That is the core of it. If it doesn't make sense for a ratio unit to have more than one numerator or denominator, we could codify that in the formal definition of gist:RatioUnit. This is what I have in mind:

# Amend definition of gist:RatioUnit - switch someValuesFrom to allValuesFrom and add cardinality constraints

gist:RatioUnit
	a owl:Class ;
	owl:equivalentClass [
		a owl:Class ;
		owl:intersectionOf (
			gist:UnitOfMeasure
			[
				a owl:Restriction ;
				owl:onProperty gist:hasDenominator ;
				owl:allValuesFrom gist:UnitOfMeasure ; # Change from someValuesFrom to allValuesFrom
			]
			[
				a owl:Restriction ;
				owl:onProperty gist:hasNumerator ;
				owl:allValuesFrom gist:UnitOfMeasure ; # Change from someValuesFrom to allValuesFrom
			]
                        [
				a owl:Restriction ;
				owl:onProperty gist:hasDenominator ;
				owl:cardinality 1 ; 
			]
			[
				a owl:Restriction ;
				owl:onProperty gist:hasNumerator ;
				owl:cardinality 1 ;
			]
		) ;
	] ;

# Amend proposed definition of gist:CurrencyPerDurationUnit

gist:CurrencyPerDurationUnit
	a owl:Class ;
	owl:equivalentClass [
		a owl:Class ;
		owl:intersectionOf (
			gist:RatioUnit
			[
				a owl:Restriction ;
				owl:onProperty gist:hasDenominator ;
				owl:allValuesFrom gist:DurationUnit ; # Change from someValuesFrom to allValuesFrom
			]
			[
				a owl:Restriction ;
				owl:onProperty gist:hasNumerator ;
				owl:allValuesFrom gist:CurrencyUnit ; # Change from someValuesFrom to allValuesFrom
			]
		) ;
	] ;


Copy link
Contributor

Choose a reason for hiding this comment

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

That is the core of it. If it doesn't make sense for a ratio unit to have more than one numerator or denominator, we could codify that in the formal definition of gist:RatioUnit.

Yes, that would make sense. Note that it's possible to have two equivalent units with different numerators and denominators. For example, there might be a currency unit for 100USD and a DurationUnit of 100 Days. The Ratio unit 100USD/100Days is equivalent to the ratio unit USD/day, but each has different numerator and denominator. But even here, a single unit still has only one numerator and one denominator.

@rjyounes
Copy link
Collaborator Author

rjyounes commented May 19, 2023

@uscholdm @dylan-sa This looks correct, but this is not the place to redefine RatioUnit. Are these definitions correct in that they follow current practice in defining magnitude and unit classes? This is what needs to be reviewed. It will cause confusion if one definition diverges from the others: it will make people start wondering why this one is different.

Please feel free to open another issue for the larger topic.

@rjyounes rjyounes closed this May 19, 2023
@rjyounes rjyounes deleted the ryounes/issue817_monetary_per_duration branch May 19, 2023 13:15
@rjyounes
Copy link
Collaborator Author

This was closed due to technical issues related to branch names. It's been recreated as #856.

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.

Add magnitude and unit classes for monetary-per-duration
3 participants