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

Rename Converter.isLinear() or change its specification #39

Closed
desruisseaux opened this issue Apr 10, 2017 · 13 comments
Closed

Rename Converter.isLinear() or change its specification #39

desruisseaux opened this issue Apr 10, 2017 · 13 comments

Comments

@desruisseaux
Copy link
Contributor

desruisseaux commented Apr 10, 2017

The Converter.isLinear() definition given in Javadoc basically restricts "linear" conversions to this formula:

y = a*x

The definition given in Converter.isLinear() does not allow the following formula, despite that formula being usually considered linear too:

y = a*x + b

So conversions between Celsius and Kelvin (or between Celsius and Fahrenheit) are not linear according the definition given in Converter.isLinear() javadoc, which I found surprising. It seems to me that the "isLinear" name is not appropriate for that method. Maybe "isScale" would have been more accurate.

Needs #95

@keilw
Copy link
Member

keilw commented Apr 10, 2017

Unless this can be addressed in JavaDoc only, it would be a breaking change to the API. Even for a MR we'd have to be extremely careful now that the API is Final. Adding e.g. the suggested isScale or a similar one in an MR might work, but nothing can be renamed or removed without deprecating it for several versions. Backward-compatibility is important.

@desruisseaux
Copy link
Contributor Author

desruisseaux commented Apr 11, 2017

Yes, I know that backward compatibility is important. But currently either the method name is misleading or the method definition is in disagreement with the usual meaning of "linear". An alternative to method renaming would be to change the method specification to comply with the usual definition of "linear", but it would also be an incompatible change (in method behavior).

@keilw keilw added the deferred label Apr 11, 2017
@keilw
Copy link
Member

keilw commented Apr 11, 2017

Unless any of that could simply be described in JavaDoc, even changing the Spec document would require a MR. More substantial changes to JavaDoc, API or at least JavaDoc and similar content is likely to come up in Spring 2018 (Before the CGPM 2018 (http://www.bipm.org/en/measurement-units/rev-si/) so let's keep those changes in mind and apply them when the SI standard is also meant to change.

@andi-huber
Copy link
Member

In general a UnitConverter represents a transformation function x->f(x), where x and f(x) are in real number space.

Currently we use UnitConverter.isLinear() in indriya as test for whether the transformation function f is a scaling transformation and thus commutes with other scaling transformations when composing multiple transformations.

I agree that UnitConverter.isScaling() is a better naming.

We might also consider introducing UnitConverter.characteristics() instead, to return a set of enums, one of which could be declared SCALING_TRANSFORMATION or similar.

Other useful characteristics for UnitConverter that come to mind are

  • BIGINTEGER_CALCULUS ... stating that the converter supports BigInteger calculus
  • BIGDECIMAL_CALCULUS ... stating that the converter supports BigDecimal calculus

See also Java's Characteristics for Collector ...
https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collector.Characteristics.html

@filipvanlaenen
Copy link

Should the name be isScalar()?

@keilw keilw added the prio:2 Priority 2 label Oct 1, 2018
@keilw keilw added question and removed deferred labels Oct 24, 2018
@keilw keilw added prio:3 Priority 3 and removed prio:2 Priority 2 labels Nov 3, 2018
@keilw
Copy link
Member

keilw commented Mar 5, 2019

@desruisseaux, @filipvanlaenen, @andi-huber, @thodorisbais (you are probably newer to this thread) there has been little activity over the last year. How strong do you feel about this change? It would be better to get it into 2.0 and deprecate the old name if renaming was found very crucial. Otherwise maybe some clarification in the JavaDoc would do (which could also go into a MR)

@andi-huber
Copy link
Member

andi-huber commented Mar 5, 2019

If we agree that UnitConverter represents a transformation in the mathematical sense, than (if I understand correctly) the predicate UnitConverter.isLinear() and its current use (check f. commutativity with other 'linear' UnitConverters) is perfectly in line with the definition of a Linear Transformation [1]. Which justifies its current naming. The Javadoc however needs to reflect that as well.

So from my point of view the name should stay, unless you intend a different use, other than a check for commutativity.

[1] http://mathworld.wolfram.com/LinearTransformation.html

@desruisseaux
Copy link
Contributor Author

We can keep the name as-is with a clarification in the javadoc saying that it is slightly different meaning than "linear equation". The link given by Andi seems to match, indeed.

@keilw
Copy link
Member

keilw commented Mar 5, 2019

Thanks, then the priority is a bit less critical but of course it would be nice to have it with 2.0

@teobais
Copy link
Member

teobais commented Mar 10, 2019

I arrived here a bit late, but...do we already have a decision?

If I'm not mistaken, we keep the name as is and we just try to adjust the javadoc to the link Andi sent, right?

@teobais
Copy link
Member

teobais commented Mar 11, 2019

Closed with #181 .

@andi-huber
Copy link
Member

PR #182 created

@keilw
Copy link
Member

keilw commented Mar 11, 2019

I saw it, thanks, I asked @desruisseaux to have a look at it, hope he gets a chance.

andi-huber added a commit to andi-huber/unit-api that referenced this issue Mar 20, 2019
keilw added a commit that referenced this issue Mar 20, 2019
#39: improving Javadoc for UnitConverter#isLinear and #isIdentity
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

5 participants