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

Arithmetic operations should be done in system units for certain scales #128

Closed
filipvanlaenen opened this issue Aug 6, 2018 · 53 comments
Closed

Comments

@filipvanlaenen
Copy link
Collaborator

filipvanlaenen commented Aug 6, 2018

Implementation of unitsofmeasurement/unit-api#95 and unitsofmeasurement/unit-api#131 in Indriya.

Some examples of the expected behavior:

  • 0 ⁰C + 0 ⁰C = 273.15 ⁰C
  • 0 ⁰C × 0 ⁰C = 74,610.9225 ⁰C²
  • 0 ⁰C × 2 = 273.15 ⁰C
  • 273.15 ⁰C / 0 ⁰C = 2 one
  • 0 ⁰C / 2 = -136.575 ⁰C

Needs unitsofmeasurement/unit-api#95
Also see https://github.com/unitsofmeasurement/unit-api/wiki/Arithmetic-rules-for-Difference-versus-Absolute-quantities

@filipvanlaenen filipvanlaenen self-assigned this Aug 6, 2018
@keilw
Copy link
Member

keilw commented Aug 6, 2018

I certainly would not do that for every single operation. It is a burden and hassle having to convert 1mi+5in to metre first and then back (to WHAT?) So doing this requires the distinction between the "level" of unit/quantity (Nominal, Ordinal, Interval,...) as in 4. of the last check-list in #95.

If you plan to do this for Temperature only, why not, but please let's wait till all the items are worked on in a particular order.

@desruisseaux
Copy link
Contributor

Actually it does not depends on the level of quantity (Interval versus Ratio). In current state of Unit API, all Quantity instances are assumed to have Ratio level, which is why conversion to system unit is conceptually required for all of them (not only temperature), skipping that conversion only when the implementation can determine that the result would be equivalent. Specifying the level of quantity is a separated issue.

This issue is already fixed in Apache SIS implementation of units of measurement without additional burden and without the need to perform any special case for temperature. I will try to copy the code to seshat this week.

@keilw
Copy link
Member

keilw commented Aug 6, 2018

Well then let's park this till we see, how Seshat deals with it. The users do expect the same behavior for a majority of units, so a 1mi+5in calculation should be in MILES until we introduced something like a target unit for that calculation.

@keilw keilw added the analysis label Aug 6, 2018
@keilw keilw added this to the 2.0 milestone Aug 7, 2018
@keilw
Copy link
Member

keilw commented Aug 7, 2018

Please note, while scheduled for 2.0 of course, both this ticket and #124 (which is currently considered prio1, but there are other UnitFormat implementations that can deal with it, so there is a workaround and it only occurs with newly introduced labelled units) should either be solved within 2-3 weeks, otherwise we can't have it in EDR1. And the EDR must be published within 9 months, which is reached in September, so no later than the last week of August everything else goes into the Public Review stage (that is another 6 months later, we should aim for end of the year to be ready for the SI reforms in early 2019)

@filipvanlaenen
Copy link
Collaborator Author

I found out that we probably have to do the arithmetic in BigDecimal (both for correct precision and overflow handling) and with some conversion anyway, and since I'm moving this code into a single class, implementation would be relatively simple. I had it working for multiplication (quantity × quantity), but haven't had time to check it in and create a pull request.

@keilw
Copy link
Member

keilw commented Aug 7, 2018

Are any other PRs or tickets related to this one? Have not tried for a PR, but issues in Waffle can be marked as needs #X or requires #Y (only in the first description field), so they are connected in the Waffle Kanban board.

@keilw keilw added the prio:2 Priority 2 label Sep 14, 2018
@keilw keilw added the function label Oct 3, 2018
@keilw keilw added the to do label Oct 30, 2018
@keilw keilw changed the title Arithmetic operations should be done in system units Arithmetic operations should be done in system units for certain measurement levels Oct 30, 2018
@keilw keilw changed the title Arithmetic operations should be done in system units for certain measurement levels Arithmetic operations should be done in system units for certain scales Dec 18, 2018
@desruisseaux
Copy link
Contributor

Note: when Wikipedia said that multiplication of an absolute measurement in Celsius is not allowed, my interpretation is that the multiplication is not allowed with the value as-is (in Celsius). There is nothing saying that we are not allowed to convert the quantity in order to carry the operation.

Multiplying a Quantity expressed in CELSIUS is not forbidden. What is forbidden is doing multiplication without prior conversion to ratio units for some scales. The scale is used for determining how to convert to KELVIN before the multiplication:

  1. If absolute: convert 2°C as 275.15 K, then multiply.
  2. If difference: convert 2°C as 2 K, then multiply.

Implementation is free to convert the result back to °C if they wish. My proposal is to keep the result of case 1 above in kelvin for less surprising behavior. Case 2 can be converted back to Celsius with no harm.

@teobais
Copy link
Member

teobais commented Feb 10, 2019

After discussion with @keilw and given the comments above, it looks like this is one of our major stories for the near future.

@filipvanlaenen I see you mentioned that you've already started working on this. Do we have any update?

@keilw
Copy link
Member

keilw commented Feb 10, 2019

@thodorisbais Thanks for offering to help. @filipvanlaenen has assigned this to himself, so if you were able to help him where necessary, it would be great. The 2.0 milestone tickets that are still open are https://github.com/unitsofmeasurement/indriya/milestone/1. While Compound Formatting already works as intended, parsing is to be done. I should be able to do the lion share of the format epic. Beside that "function" is where this ticket resides, there is also something to do in the "documentation" epic, e.g. JavaDoc, but some of it has already been done.

@magesh678
Copy link

@thodorisbais let me know if you need my helping hand in this implementation.

@magesh678
Copy link

Is this typo mistake
* 0 ⁰C + 0 ⁰C = 273.15 ⁰C
I think it should be
* 0 ⁰C + 0 ⁰C = 273.15 ⁰K

How will we know the target unit to be converted if it is not part of input?
Will this apply to all unit types (where two type conversion possible - eg: Cel ->Far, Mile -> KM, Mtr -> KM)

@desruisseaux
Copy link
Contributor

It is not a typo. 0°C + 0°C = 275.15 K + 275.15 K = 550.3 K = 275.15 °C (note: there is no "°" in front of Kelvin units). We know this is counter-intuitive.

About what should be the target units: I don't think it needs to be specified. It can be implementation choice, as long as the result is equivalent. For example 550.3 K is equivalent to 275.15°C, so both are acceptable output. But 275.15 K is not equivalent to above measurements, so it is not allowed.

@teobais
Copy link
Member

teobais commented Mar 2, 2019

@thodorisbais let me know if you need my helping hand in this implementation.

@magesh678 as you can see (in the discussion above and in the assignees' section of the right top of this issue), Filip is the one who's working on it, so, I guess the question regarding any help should be addressed to him. :-)

That said, @filipvanlaenen , do you need any help here? :-)

@magesh678
Copy link

Thanks @thodorisbais . @filipvanlaenen Do you need any help here?

@teobais
Copy link
Member

teobais commented Mar 11, 2019

Maybe I/it start/s becoming annoying, but given that our PR is soon, I need to ping @filipvanlaenen again for the status of this issue :-) . Any news, Filip?

@teobais
Copy link
Member

teobais commented Mar 19, 2019

@filipvanlaenen any news here?

@teobais
Copy link
Member

teobais commented Apr 2, 2019

Just a few days left until Apr. 19, so, I need to ask again :-) : @filipvanlaenen any news here? How is this going? Do you need any help?

@desruisseaux
Copy link
Contributor

I'm not sure there is need for action (or maybe I have missed the point). What are the choices on the table?

Regarding above discussion about whether to allow or disallow 1°C + 1 K for example, I have no objection to leave the choice to implementations. My only requirement is that if an implementation chooses to allow those operations, then the result shall be as if all calculations were done in ratio units (taking in account the RELATIVE / ABSOLUTE nature of the quantity) before to convert the final result in whatever units the implementation wants.

@keilw
Copy link
Member

keilw commented Jun 7, 2019

Well this is the Reference Implementation, so unless we suggest that Quantity.add() and other operations shall be overriden by applications with special requirements, it would also have to be solved in this implementation, even if it was just for CELSIUS and KELVIN (because extensions like FAHRENHEIT had to be treated differently) We have a few other local "hacks" primarily because both KILOGRAM and GRAM are present to properly format these units.

@dstibbe
Copy link

dstibbe commented Jun 10, 2019

@desruisseaux I agree with most of what you wrote, except for a small point:
0C + 0C = 273.15C makes the assumption that both (0C) are absolute values and not interval values.
If someone walked a distance 1 Plumbus (started at 10, ended at 11 Plumbus), and then another distance of 1 Plumbus (started at 11, ended at 12 Plumbus), he has most definitely traveled a distance 2 Plumbus. Because the context clearly indicates we are talking about 'interval's here and thus must treat them as such.
Now if you would say: what is 10* 3 plumbus, the answer could be either 30 plumbus (10 times the 3 plumbus interval) or -600 plumbus (10 times the location of 3 plumbus on the scale). This is therefore a calculation I think should only be allowed if it is clearly indicated what should be considered at interval scale and what should be at ratio scale.

I like the quote on wikidedia (https://en.wikipedia.org/wiki/Celsius) :
"Because of this dual usage, one must not rely upon the unit name or its symbol to denote that a quantity is a temperature interval; it must be unambiguous through context or explicit statement that the quantity is an interval."

Whatever is chosen, I'd say, if there is a unit with an interval scale involved, have the user be explicit about how he wants it to be evaluated. e.g.

2<C, interval> + 2<C, interval> = 4 <C, interval>       = 4 <K>
2<C, interval> + 2<K>               = 4 <C,interval>        = 4 <K>
2<C, ratio> + 2 <C, interval>     = 4 <C, ratio>           = 277.15  <K>
2<C, ratio> + 2 <C, ratio>          = 277.15 <C, ratio>  = 550,3  <K>
2 <C,ratio> + 2 <K>                   = 4 <C, ratio>           =  277.15  <K>

2<C,interval> * 2<K>                 =  4 <C, interval>      = 4<K>
2<C,ratio> * 2<K>                      = 277,15 <C,ratio>    = 550,3 <K>

and thus, given U_i is the unit with an interval scale (e.g C), U the corresponding ratio scaled unit (e.g. K) and b the distance of the origin of U_i from the U scale (e.g. 273,15):

x <U_i, interval>                                 = x <U>
x <U_i, ratio>                                     = b+x <U>

x <U_i, interval> + y <U_i, interval>  = (x+y) <U>               = (x+y) <U_i, interval>    
x <U_i, interval> + y <U>                  = (x+y) <U>               = (x+y) <U_i, interval>   
x <U_i, ratio> + y <U_i, ratio>          = 2b + (x+y) <U>        = b + (x+y) <U_i, ratio>  
x <U_i, ratio> + y <U_i, interval>      = b + (x+y) <U>         = (x+y) <U_i, ratio>        

v* x <U_i, ratio>                                  = (v* (b +x)) <U>       = (v*b + v*x - b) <U_i, ratio>
x <U_i, ratio> * y <U_i, ratio>             = (x+b)(y+b) <U>      = (x+b)(y+b) - b <U_i, ratio>
x <U_i, ratio> * y <U_i, interval>         =  y*(x+b) <U>          =  y*(x+b) - b <U_i, ratio>  
x <U_i, interval> * y <U_i, interval>    =  y*x <U>                 =  y*x <U_i, interval>   
x <U_i, ratio> * y <U>                        =  y*(x+b) <U>           = (y*b + y*x - b) <U_i, ratio>      
x <U_i, interval> * y <U>                    =  y*x <U>                 =  y*x <U_i, interval>   

PS.
Thanks for the discussion and your patience. This has been very educational for me.

@desruisseaux
Copy link
Contributor

0C + 0C = 273.15C makes the assumption that both (0C) are absolute values and not interval values.

True, but if the user did not said explicitly if the values are interval or absolute measurements, we have to make some assumptions. In past debates we did not found an arithmetically consistent assumption other than assuming that all measurements are absolute (thread of past debates contain equations showing inconsistencies of other assumptions).

For making possible to get 0°C + 0°C = 0°C, we need to explicitly specify that at least one measurement is an interval. The equations that you wrote are fine and illustrate well this need: an information about ratio versus interval scale is attached to every measurements. The currently proposed way to provide this information in JSR-363 is the ABSOLUTE versus RELATIVE enumeration. But the principle is similar.

@keilw
Copy link
Member

keilw commented Jun 10, 2019

No it's not provided by JSR-363, it is by JSR-385 however ;-)

@desruisseaux
Copy link
Contributor

Ah, sorry! Thanks for the correction.

@keilw
Copy link
Member

keilw commented Jun 11, 2019

Please have a look at https://github.com/unitsofmeasurement/indriya/blob/master/src/test/java/tech/units/indriya/quantity/AbsoluteVsRelativeTest.java and https://github.com/unitsofmeasurement/indriya/blob/master/src/test/java/tech/units/indriya/quantity/WolframTuturialTemperatureTest.java based on the Wolfram statements about calculating Temperature quantities. Except 2 all others are met already.

If there are specific operations that need tweaking or extension, I guess an SPI approach similar to addressing #229 (a detail of Quantity arithmetics from a numeric precision point here, the NumberSystem can also be extended and exchanged via SystemLoader) is worth a thought. It may cover the most basic use cases like CELSIUS and KELVIN out of the box but allow extending it e.g. if further units are introduced by other systems of units.

@teobais
Copy link
Member

teobais commented Jun 11, 2019

I need a better look at this one so that I can express an opinion. Hopefully, that's gonna happen during the weekend. Good discussions, though, people!

@dstibbe
Copy link

dstibbe commented Jun 12, 2019

@desruisseaux and that was just my point: don't make assumptions when dealing with units that are non-ratio-scale (e.g. C). Let the user be explicit. Assumptions are what made the spacecraft crash in the first place ;-)

@keilw the tests look great. But like my comment to @desruisseaux , would it be an idea to make RELATIVE/ABSOLUTE mandatory for non-ratio-scale units ?

This is my last comment on this subject :P I think I have bothered you guys enough by now. Thanks for the patience.

@keilw
Copy link
Member

keilw commented Jun 12, 2019

@dstibbe RELATIVE/ABSOLUTE is already mandatory (unless explicitly set, quantities are ABSOLUTE because it is the most common case), we decided against the "ratio" or other LevelOfMeasurement earlier. It would be a bit of a challenge to introduce it at this stage because the Public Review is practically code-freeze, unless we came across a convincing reason to bring it back. Only then we could have a distinction between RATIO and non-ratio, do you think that could help for a more generic solution?

@desruisseaux
Copy link
Contributor

Talking about Public Review, is there a place where we can comment? More specifically I'm a little bit confused about the src/etc/modules/ directory (but I guess that this is the topic of another issue than this page).

@keilw
Copy link
Member

keilw commented Jun 12, 2019

There is no such directory in the RI, and for optional smaller modules there is this ticket: unitsofmeasurement/unit-api#42 It is not subject to the main deliverables but if a tiny box with very constrained memory (e.g. Raspberry Pi Zero or similar) needs a tailor-made version of the API they can use these profiles. And likely have to create a smaller implementation (like https://github.com/unitsofmeasurement/uom-impl-enum) Again, we're not talking about the deployables to MavenCentral, this is for Embedded use cases where they only need one or two modules for a particular profile.

@keilw
Copy link
Member

keilw commented Jun 13, 2019

Coming back to the original problem here, @dstibbe thanks a lot for the detailed listing of possible combinations. The most important question is, can Scale.ABSOLUTE and Scale.RELATIVE work as substitute for what you called RATIO and INTERVAL? Because any Quantity can be created with a scale, so it is of course possible to tell them apart where needed.
Simply renaming the Scale or even adding the other 2 (we discussed it earlier and boiled it down to just those two) into the enum would also work, it was of course an API change but since we have only limited effects of that Scale right now, it could still be possible but it must be prior to 2.0 Final stage.

https://en.wikipedia.org/wiki/Conversion_of_units_of_temperature#Comparison_of_temperature_scales contains a good comparison of temperature scales. It does not talk about RATIO or INTERVAL there but an "absolute Zero" which is partly why we ended up with ABSOLUTE (e.g. RANKINE and KELVIN) vs. RELATIVE (all others in most cases)

However taking the last two cases

2<C,interval> * 2 = 4 <C, interval> = 4
2<C,ratio> * 2 = 277,15 <C,ratio> = 550,3

It just does not seem right if you took

2<ft,interval> * 2 = 4 <ft, interval> = 4
2<ft,ratio> * 2 = 0,3048 <ft,ratio> = 0.6096.

Taking a forced conversion into the system unit (METRE) for the lower example, even if you got the mathematically correct value of 0.6096 m does not make sense because we must not force-convert ft into m unless a user or their app really wants to do that conversion.

So all we know at the moment at runtime is the Quantity.Scale and the distinct Unit either CELSIUS, KELVIN or otherwise.

@teobais
Copy link
Member

teobais commented Jun 13, 2019

Given that 1. the discussions are escalating to a completely different level, and 2. Filip has not provided any update here for a while, it's wise to remove Filip as the assignee of this issue so that we give the chance to anyone else who might be interested in taking this over.

In case Filip comes back with an update, we can always rediscuss it :-) .

@keilw
Copy link
Member

keilw commented Jun 13, 2019

Thanks, the ticket already has "Help needed" and from what I recall Filip expects a new member of his family, so I guess he could be busy for a good reason right now ;-)

@dstibbe
Copy link

dstibbe commented Jun 13, 2019

Previous equations

To start, in my previous set of equations, the <K> was not shown, I've fixed it by making it a code block.
So:

2<C,interval> * 2 = 4 <C, interval> = 4
2<C,ratio> * 2 = 277,15 <C,ratio> = 550,3

was actually:

2<C,interval> * 2 = 4 <C, interval> = 4 <K>
2<C,ratio> * 2 = 277,15 <C,ratio> = 550,3 <K>

A small difference, but not unimportant. The K unit is very relevant. I have fixed the previous post.

@keilw Regarding your remark about force convert; I absolutely agree with you. The = 4<K> in the above was meant to be read as == 4<K>.
It was merely meant to illustrate equality, not conversion.

Definitions

Let's start with terminology, I noticed that I myself have not been as consistent as I would like to be
(one reason being that I've learned a lot during this conversation).

Units have a:

  • Absolute scale = a scale that has a 'true' zero-point as origin (there is nothing lower) in a Dimension, (consequently it is always in a ratio scale), e.g. K
  • Relative scale = a scale that has an arbitrary origin in a Dimension, e.g. C, , these have a translation from an absolute scale, e.g. +273,15
  • Size = the magnitude of a single unit

Quantities are

  • measurement = the quantity lies on the scale as defined by the unit, e.g. 1C = 274,15 K
  • difference = for the degree of difference between two measurements, e.g. 11C - 10C = 1C = 1K

If one disagrees with these definitions, don't mind correcting me. I could find no proper alternative for the above term 'measurement'
in the context of this discussion.

Temperature

Following these definitions, the units for C and K are, in pseudo code:

K = AbsoluteUnit( "K" , Temperature)
C = RelativeUnit( "C" , K + 273,15 )

providing quantities for K is done as following:

myQ = getQuantity( "3", "K")

Since K is in absolute scale (AbsoluteUnit), there is no need to indicate the type of quantity.

When providing quantities for C we should require(!) the user to provide an indication of the type of Quantity, because it is a RelativeUnit and
the outcome really depends on it:

getQuantity( "3", C , MEASUREMENT) - getQuantity( "2", C , MEASUREMENT) === getQuantity( "1", C , DIFFERENCE)   <-- attention: difference
getQuantity( "3", C , MEASUREMENT) - getQuantity( "2", C , DIFFERENCE)  === getQuantity( "1", C , MEASUREMENT)  <-- attention: measurement
getQuantity( "3", C , MEASUREMENT) - getQuantity( "2", K)               === getQuantity( "274,15", K) 
getQuantity( "3", C , MEASUREMENT) + getQuantity( "2", C , MEASUREMENT) === getQuantity( "278,15", C , MEASUREMENT) 
getQuantity( "3", C , MEASUREMENT) + getQuantity( "2", C , DIFFERENCE)    === getQuantity( "5", C , MEASUREMENT) 
getQuantity( "3", C , MEASUREMENT) + getQuantity( "2", K )              === getQuantity( "278,15", K) 
getQuantity( "3", C , DIFFERENCE)  - getQuantity( "2", C , DIFFERENCE)  === getQuantity( "1", C , DIFFERENCE) 
getQuantity( "3", C , DIFFERENCE)  - getQuantity( "2", K )               === getQuantity( "274,15", K)  
getQuantity( "3", C , DIFFERENCE)  + getQuantity( "2", C , DIFFERENCE)  === getQuantity( "5", C , DIFFERENCE) 
getQuantity( "3", C , DIFFERENCE)  + getQuantity( "2", K )               === getQuantity( "278,15", K) 

or

xM - yM = (a(x) - a(y))  D         
xM - yD = (x - y)        M         
xM - yB = (b(x) - y)     B       
xM + yM = m(b(x) + b(y)) M
xM + yD = (x + y)        M
xM + yB = (b(x) + y)     B
xD - yD = (x-y)          D
xD - yB = (b(x) - y )    B

where 
M: a measurement quantity in specified RelativeUnits (e.g. C)
D: a difference quantity in specified RelativeUnits (e.g. C)
B: base (which has an absolute unit scale) (e.g. K)
m(): function for converting a base quantity to a measured quantity 
b(): function for converting a measured quantity to a base quantity 

Note:  the above only works with the sizes of the both units being the same (e.g. a C is just as big as a K). If they differ, then the difference quantities
should be converted with a function as well (see: Conversion).

@keilw
Interesting to see is that when K comes into the equation, the result will be K, and the information regarding DIFFERENCE/MEASUREMENT is lost
, since K in the equation had none either. When the K quantity does provide that information, it would also influence the quantity type of the outcome.

Length

Both ft and m are AbsoluteUnit's in the Length dimension. Unlike K and C, they do not differ in origin. However, where K and C are equal
in size, ft and m are not.

That means that ft and m would not require a quantity type, since they are both AbsoluteUnits:

getQuantity("2" ,ft> * 2 = 4 <ft> === 1.2192 <m>

We can use MEASUREMENT/DIFFERENCE, but it is not required.

Conversion

How to convert between Relative and Absolute units?

Let's take Plumbus once more :) Now let's redefine the Plumbus (Plb) to have an origin at -200 m and have a Plb be the size of 13 m

In pseudo

Plb = RelativeUnit("Plb", PlbTranslation(-200,m), PlbSize(13,m) )

The conversion between Plb and m would be done as following:

Measurement:
myMeasurementInPlb = getQuantity( "5", Plb, MEASUREMENT)

myMeasurementInM = myLengthInPlb*PlbSize - PlbTranslation === getQuantity( 5*13-200, m) == getQuantity( -135, m) 


Difference:
myDifferenceInPlb = getQuantity( "5", Plb, DIFFERENCE)

myDifferenceInM = myLengthInPlb*PlbSize === getQuantity( 5*13, m) == getQuantity( 65, m) 

Same unit, different origin

Lets say you want to do some logic with measurements of the height of the water. In the Netherlands this is done relative to the NAP (Amsterdam Ordnance Datum).

How do we handle that? It is actually irrelevant what the actual height of the NAP is, as long as all are measured alongside it.

We can't say NAP = RelativeUnit( "m" , M + ??? ), because a) the unit symbol is the same ('m') and b) we have no idea about the offset.

I would propose something along the lines of:

NAP = Reference("m")

And Length measurement quantities can be given according to the NAP reference:

diffUtrechtAmsterdam = getQuantity( "5", m, NAP) - getQuantity( "2", m, NAP) === getQuantity( "3", m, DIFFERENCE)
napUtrecht = getQuantity( "2", m, NAP) + getQuantity( "3", m, DIFFERENCE) === getQuantity( "5", m, NAP)

But the following would be invalid:

getQuantity( "5", m, MEASUREMENT) + getQuantity( "5", m, NAP) 

getQuantity( "5", m, NAP) + getQuantity( "5", m, SOME_OTHER_REFERENCE) 

Since we cannot add NAP to a measurement if we do not know the offset. And we can't add NAP to a measurement of some other reference.


@keilw
Anyway... those are my 2 cents (feels more like a whitepaper somehow).
I think ABSOLUTE/RELATIVE are fine as they are now (instead of the MEASUREMENT/DIFFERENCE). But I think they would not cover all scenarios described above.
The above would take a shitton of testing to get it right, if at all. So it would not be sensible to implement now.

@dstibbe
Copy link

dstibbe commented Jun 13, 2019

Given that 1. the discussions are escalating to a completely different level, ...

Sorry about that :S

@keilw
Copy link
Member

keilw commented Jun 14, 2019

@dstibbe, @thodorisbais No need to be sorry, that's exactly what a Review is for ;-)
Thanks for the constructive input. Adjusting ABSOLUTE or RELATIVE scales based on above discussions and guidelines should be rather easy.
For other cases we may have to define tests and see, how to best apply.

@teobais
Copy link
Member

teobais commented Jun 18, 2019

Hey @mthoolen, welcome!
Screenshot 2019-06-18 at 21 12 52

First things first, so, just a few words about the logistics:

  • serving JSR-385 as a contributor means that basically, you can contribute code to this JSR (we normally do this via PRs)
  • in addition, you have all the freedom to help with the architecture, etc., so, indeed, as @keilw mentioned during our Adopt-a-JSR Day, having joined as a Contributor grants you 100% contribution rights
  • you will soon be added in the @unitsofmeasurement/contributors list as well
  • should you have any further questions, we're always here

On topic, what is your take on this one (, given that you have already been exposed to it during our Adopt-a-JSR Day)?

@mthoolen
Copy link
Contributor

Thanks @thodorisbais, lots to read up on!

Looking at @dstibbe 's comment, I agree with what he said. During the Adopt-a-JSR day I worked on this issue and found a fix for adding relative temperatures in anything but Kelvin. It was based on only converting the value of the quantity if the quantity was relative, or a difference as @dstibbe calls it, and if addition is used in converting. I believe only converting for the ratio (or size?) between two units if it is a relative quantity (or difference) and not for the offset in origin would lead to the right numbers at least.

How do we proceed from here? I can offer my fork for a pull request if you'd like, even though I've only changed the behavior for addition at the moment.

@keilw
Copy link
Member

keilw commented Jun 20, 2019

@mthoolen Yes, if you could propose a pull request, that would be great. We could then have others review it (including @dstibbe although I'm not sure if he's a JCP member but making comments here or reviewing a PR is open to everyone, only those who raise the PR or merge it should be JCP members) to get an outcome everyone feels works best.

@dstibbe
Copy link

dstibbe commented Jun 21, 2019

@keilw I'd be happy to review it. I'd even be happy to make a PR, but I am indeed not yet a JCP member. I did sign the JCP Full Membership Agreement, but it is waiting for review.

@keilw
Copy link
Member

keilw commented Jun 21, 2019

Cool, no problem to review PRs by others in the meantime. As the PMO accepted somebody else's EG membership for JSR 385 quite recently, should your Full membership get approved within a few weeks rather than months, we could still offer you a full EG membership if you want. Helping this rather sophisticated ticket alone justifies that IMO ;-)

keilw added a commit that referenced this issue Jun 25, 2019
Added behaviour for adding values with an offset origin (issue #128)
@keilw
Copy link
Member

keilw commented Jul 7, 2019

Closing this, created #247 as a follow up for some lose ends.

@keilw keilw closed this as completed Jul 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants