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

Base overrides #9

Closed
timholy opened this issue Jul 23, 2016 · 38 comments
Closed

Base overrides #9

timholy opened this issue Jul 23, 2016 · 38 comments

Comments

@timholy
Copy link
Contributor

timholy commented Jul 23, 2016

I'm rewriting Images.jl, which currently (sort of) supports encoding of information like the spacing between pixels with unit-ful quantitites (currently via SIUnits). In the rewrite (which will require julia 0.5), I'm struggling with whether to remain with SIUnits or switch to Unitful. The switch faces a number of issues:

  • while Unitful may be more up-to-date than SIUnits, base julia has nevertheless changed considerably since it was first written: Functors have been removed, and the whole promote_op infrastructure has been dramatically simplified with (yay!) finally the ability to rely on inference for many operations
  • you override many functions in base. The number of warnings is a significant disincentive for me to switch.
  • there were some good discussions in RFC: Modifying base to play nicely with physical units JuliaLang/julia#15394 about potential problems with ranges, but those seem to have remained unresolved.
  • this is not registered as a package, which would also be a prerequisite for switching.

I guess I'm posting this largely to try to gauge your enthusiasm for updating Unitful and addressing these issues. Given the number of other packages I maintain/work on I probably don't have the bandwidth to make major changes to this package (nor to SIUnits), so I am trying to read the tea leaves to figure out what will become the future well-supported units package in Julia.

@ajkeller34
Copy link
Collaborator

I'm certainly interested in updating Unitful and addressing these issues. Of course, I would be delighted to have this package be used broadly.

To be honest about my bandwidth, I am doing an experimental physics postdoc, with all that entails. That being said, the initial reason I wrote Unitful in the first place was because I figured that Keno Fischer would be busy or more interested in working on higher profile projects like Gallium or Cxx for the foreseeable future. I wanted units functionality for my measurement and CAD code, and it didn't seem like SIUnits would be updated anytime soon. That was just my reading of the tea leaves, and I could be wrong. Do you have some timeline in mind for when you would want Unitful to be stable and fully functional?

Here are some replies to your concerns:

  • I'll take a look at my code and see if I can deduce what needs to be rewritten. Please do let me know if you have identified any specific places that need updating.
  • I agree that the number of warnings is irritating. I know you don't have the bandwidth to make major changes to this package, however, if you did have the bandwidth to work with me specifically on modifying base to enable at least some subset of the range types to work with units, we could get rid of the warnings. Or, if it means broader adoption and long-term interest, I am fine with temporarily dropping support for the features that override functions in base until the requisite changes are made. This should kill all warnings. Are either of these options appealing?
  • I came to the conclusion that I would need at least one new function in base to take a quantity with units and separate the number and its unit. Otherwise, I did not know how to avoid changing the numeric type during the unit stripping when, for example, it was an integer with units (dividing by oftype(T,1) converts to a floating-point type). Apart from exact conversions between units, a specific use case for integers with units would be doing polygon clipping, etc. when the polygon coordinates have units. Some clipping algorithms use integer coordinates for numerical stability. I may have better luck convincing people of my need for a unit-stripping function in base if this package is actually used by anyone, especially someone well respected in the Julia community like yourself.
  • I am happy to register the package. Probably I should settle on the interface for the package before registering though. Is there anything you would rather be different? For instance, I could consider changing my hacky syntax for converting between units (eg. convert(µm, 100nm)). I'm also a little worried about breaking other packages if I ever rewrite how units are implemented. I could imagine needing to rewrite some of Unitful to fulfill requests I've received to enable dispatch on dimensions (e.g. x<:Length). I also don't know how well it will work if we save quantities with units to JLD files if the type implementation ever changes.

Anyway, I hope this is helpful to you regardless of which package you choose to use.

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

Thanks for your reply. Would be worth bringing @Keno into this, if he has a few CPU cycles to spare on this discussion. IMO there's little point in there being two packages with similar design, so it would be great to come to some kind of consensus.

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

BTW, at this point the thought of modifying base to make this work better is a non-starter, since 0.5 just went into feature freeze. It would have to be after 0.6 development starts, and that won't hit widespread usage for months. So realistically I think the better option is to go with what's in base and see how much of your desired functionality one can get.

@ajkeller34
Copy link
Collaborator

Alright, I'll see what breaks if I don't redefine anything in base. Probably you know already, but virtually all of what I override is for ranges. The package should still provide much functionality regardless. Perhaps we could have some kind of switch to turn on range support for eager users.

It seems like several of the tests I wrote are failing on 0.5 master. I'll investigate what has changed.

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

Seems like it should be possible to get pretty far:

julia> using SIUnits.ShortUnits

julia> r = 1m:1m:10m
1 m:1 m:10 m

julia> for i in r
           @show i
       end
i = 1 m
i = 2 m
i = 3 m
i = 4 m
i = 5 m
i = 6 m
i = 7 m
i = 8 m
i = 9 m
i = 10 m

If I remember correctly, in the discussion we had in base the problem was with UnitRange, but I think most of us think that UnitRange doesn't make any sense for something with units.

@ajkeller34
Copy link
Collaborator

Yes, SIUnits achieves ranges with units by implementing an SIRange type. My package instead enabled the native range types to work with some small tweaks in base. To me, this approach initially seemed preferable, but in any case it is now a moot point with the feature freeze. I'm happy to go the other route if it makes the majority of users satisfied.

My priority will be to fix bugs in Unitful now, particularly regarding array math. I'll work on some new range types after that.

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

I see. I hadn't looked at how it works.

To be clear, things that are effectively bugfixes should still be allowed. Changing signatures (e.g., changing LinSpace to accept any Number) is a little more dicey.

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

Looks like you don't need to do anything special:

# Let's create a "meter" type and try to build a StepRange

immutable m{T<:Real}
    coef::T
end

Base.promote_rule{S,T}(::Type{m{S}}, ::Type{m{T}}) = m{promote_type(S,T)}
Base.zero{M<:m}(::Type{M}) = M(0)
Base.zero(x::m) = zero(typeof(x))
Base.isless(x::m, y::m) = isless(x.coef, y.coef)
Base.:+(x::m, y::m) = m(x.coef+y.coef)
Base.:-(x::m, y::m) = m(x.coef-y.coef)
Base.rem(x::m, y::m) = m(rem(x.coef, y.coef))

r1 = StepRange(m(1), m(1), m(10))
r2 = StepRange(m(2.5), m(7.5), m(100.0))

Results:

julia> include("/tmp/units.jl")
m{Float64}(2.5):m{Float64}(7.5):m{Float64}(100.0)

julia> for x in r1
           @show x
       end
x = m{Int64}(1)
x = m{Int64}(2)
x = m{Int64}(3)
x = m{Int64}(4)
x = m{Int64}(5)
x = m{Int64}(6)
x = m{Int64}(7)
x = m{Int64}(8)
x = m{Int64}(9)
x = m{Int64}(10)

julia> for x in r2
           @show x
       end
x = m{Float64}(2.5)
x = m{Float64}(10.0)
x = m{Float64}(17.5)
x = m{Float64}(25.0)
x = m{Float64}(32.5)
x = m{Float64}(40.0)
x = m{Float64}(47.5)
x = m{Float64}(55.0)
x = m{Float64}(62.5)
x = m{Float64}(70.0)
x = m{Float64}(77.5)
x = m{Float64}(85.0)
x = m{Float64}(92.5)
x = m{Float64}(100.0)

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

Though it might be worth arguing to change the type declaration of LinSpace. Seems like a pretty petty restriction to take only AbstractFloat.

@ajkeller34
Copy link
Collaborator

I'm sorry but I'm sensing a bit of a mixed message. Which approach would you prefer, new range types or getting existing ones to work? I feel like I will have an uphill battle proposing changes to 0.5 at this point, especially since I haven't made a significant contribution to Julia before.

Without my base redefinitions, it looks like all my FloatRange tests fail. You're right that tests only involving StepRange work fine. However, isa((1m:2m:5m)/2, FloatRange) fails as a FloatRange is returned, so it doesn't take long to get out of safe territory.

Suppose we came up with a small list of changes that did not even change type declarations or signatures of methods, but tweaked the way things work. These changes would give the same results as now for numbers without units. Now, if these changes employ a single new function that is not exported out of base, does that count as a "new feature" or a "bug fix"?

I could do a lot if given a function that strips a number with units into the number and the unit. For numbers without units, the unit is multiplicative identity. If something like the following were in base, and used by only a small subset of the functions in base (specifically some in range.jl):

stripunits(x::Number)=(x, one(x))

Then I could write something like the following in Unitful:

import Base: stripunits
stripunits(x::Quantity)=(just_the_number(x), just_the_unit(x))

and the range magic could follow. I hope I don't sound like a broken record but I just want to get the facts on the table. I guess my philosophy with this package is that eventually, there should be little distinction between numbers with units and numbers without. Julia is for "technical computing" after all. The more special treatment we give numbers with units, the less likely they are to work generically with other packages. Rewriting ranges seems like special treatment, but I respect the rules, and if the feature freeze prevents the changes I need then I guess that's the alternative, in my view.

@timholy
Copy link
Contributor Author

timholy commented Jul 24, 2016

It is a mixed message because, from the standpoint of units, I've just realized I'm not terribly happy with most of the Range types in Base. (I just posted here: JuliaLang/julia#17408 (comment)). I would say the best approach is we should probably ignore the range types in base and start a unit-friendly LinSpace alternative. It should be a standalone package, not buried in this one, because the mathematics are very clearly broadly applicable.

I'm being a broken record too, but I think we should avoid the unit-stripping and just use the mathematics. I guess I'm of the opinion that one(x) should be the multiplicative identity for x, i.e., that the fundamental property we must satisfy is x*one(x)===x. (With the 3 =, that means even the type has to be the same.) Is that what you're intending, too?

I wonder how much in Base we assume that x + one(x) is a sensible operation (here, it's not). It would be interesting to discover what breaks when it doesn't work!

@ajkeller34
Copy link
Collaborator

Turns out StepRanges don't work entirely because of the x + one(x) issue you mentioned. Line 39 of range.jl is problematic: last = start - one(stop-start) causes 1m:10m:0m to throw an error. The offending method was overridden to something more sensible in my package. Also note my previous example, (1m:2m:5m)/2 will fail until FloatRange is fixed. So I think it is not right to characterize StepRange as working well with units after all, given how easily you can make operations with it fail.

We are definitely in agreement about the definition of one(x). It should never be used for anything but strict multiplicative identity. Everything else, in my opinion, is a total abuse and causes problems. I didn't have to look to hard to find a current issue because of abuse of one(x) (JuliaLang/julia#17314).

I'm on board if you and others want LinSpace in a separate package, but that doesn't resolve all of the patches I'd like to make to range.jl with the other types. How about if I make smaller PRs to fix bugs one at a time, starting conservatively and saving anything bordering on unit-stripping for last?

@ajkeller34
Copy link
Collaborator

I guess what I meant to say is that getting a LinSpace alternative to work with units would be great, but I think users including myself should rightfully expect unitful numbers to work with the other range types. Why shouldn't they? They are often very convenient. I basically know what needs to be fixed to make it happen, and the changes don't break anything.

@timholy
Copy link
Contributor Author

timholy commented Jul 25, 2016

I agree that the base ranges are messed up when one speaks in terms of units, and we seem to be in agreement about the meaning of one.

I guess here's the plan I'd recommend:

  • For julia 0.5, let's try to get StepRange working for Unitful. Rather than an empty range being encoded with stop = start - 1 it seems that stop = start ± step makes sense. I also think such a change would be viewed as a bugfix rather than a new feature, so that kind of thing could get in to base. I can tackle that if you like.
  • Abandon all hope of getting FloatRange and LinSpace working. Those would require a signature change, and that's less likely to fly at this point. Note that StepRange, FloatRange, and LinSpace are all the same thing, with slightly different constructors and behavior with respect to ulp-level differences, but I think for Unitful it's acceptable to get just one of them working. So the path I'd pursue is to declare those as simply broken and off-limits for Unitful numbers.
  • Solve the (1m:2m:5m)/2 problem via a specialization of either range and/or ./ for Unitful numbers, returning a StepRange instead. Note that this should yield a "closed system" of range types. You could even specialize linspace to return a StepRange.

To me it seems this plan will eliminate all override warnings when loading the package, and give you the functionality you need.

@timholy
Copy link
Contributor Author

timholy commented Jul 25, 2016

Oh, and now that I think about it, we don't even have to get the steprange_last bugfix into Base necessarily; just specialize it here for T<:RealQuantity.

@timholy
Copy link
Contributor Author

timholy commented Jul 25, 2016

JuliaLang/julia#17611

@ajkeller34
Copy link
Collaborator

All tests pass again except for the range ones that I've commented out. Also, no overriding base --> no more warnings. Let me know when you find some bugs in this thing, I'd love more tests.

@timholy
Copy link
Contributor Author

timholy commented Jul 25, 2016

Ooh! Very exciting! I will start giving it a whirl.

@ajkeller34
Copy link
Collaborator

I propose I do the following prior to publishing and tagging this package:

  1. Allow for dispatching on dimensions (x::Length), while also retaining the ability to dispatch on the appropriate abstract numeric type (x::AbstractFloat). I have a game plan for this but it will require changing the type definitions somewhat.
  2. Implement the most specific abstract numeric types for quantities with units, so that for example x::Integer will accept an integer quantity with units. Right now, it does not. The objective here is to have quantities with units behave seamlessly with existing code that expects only numbers, which may dispatch on abstract numeric types low in the Number subtype tree.
  3. In the process, replace the insane unitcount mechanism that enables new units to be created on the fly with something sane. As is, I think it will cause problems down the line when these types are saved into JLD files.

After these three changes are implemented, I expect the package's implementation to stabilize and I would be more comfortable with broader adoption.

@timholy
Copy link
Contributor Author

timholy commented Jul 29, 2016

Sounds exciting!

I'm going to be a real pest here...but, can you clarify where you see it being important to have unitful numbers be subtypes of Integer and AbstractFloat? This notion bothers me for some of the same reasons that we've already been discussing. Let's say I have a pen that's exactly 11cm long. So it's an Integer, right? Now I express the exact same quantity in inches, and it's an AbstractFloat. So which is it? No matter which units I choose to express that length in, the length is the same, yet we have a single number that's alternately being classified as an Integer or an AbstractFloat.

To me the main point is this: unitful numbers correspond to physical reality. That's very different from the artificial world in which computers work, and it's important not to let the computer world infringe unnecessarily on the actual meaning of quantities.

I'm very happy to have these be a subtype of Number. I'm skeptical even of making them a subtype of Real, because I don't think they are. They're more like complex numbers, except instead of just i you have length, time, etc. (it's a tensor-product space).

See:
http://math.stackexchange.com/questions/571050/what-mathematical-structure-models-arithmetic-with-physical-units
and its link to
https://terrytao.wordpress.com/2012/12/29/a-mathematical-formalisation-of-dimensional-analysis/

@timholy
Copy link
Contributor Author

timholy commented Jul 29, 2016

I should add that I'm happy to help with this transition. But if we simply can't see eye to eye on these matters, I certainly don't wish you to feel like you have to "break" your package just to please me---you wrote the code, and if it doesn't work for you then it's just wasted effort.

@ajkeller34
Copy link
Collaborator

First off, thank you for the offer of help. I value your feedback and you are being very generous with your time given your work on so many different projects and other responsibilities. I’ll try implementing just one Quantity that is a subtype of Number then.

I don’t think we are that different in our viewpoint—I do understand your argument (despite appearances...) and maybe it is best to keep formal rigor. My motivation for considering doing it my way was entirely practical. Let’s consider middle(x::Real, y::Real) = x/2 + y/2 in base. It is just calling some simple mathematical operations, all of which are defined for numbers with units. Now, if we just had unitful quantities be a subtype of Number only, a user wouldn't be able to call this method on real numbers with units, even though these unitful quantities are represented using some concrete subtype of Real and some unit information. This is a very pedantic example and frankly it’s not such a big deal. My worry was just that this sort of problem could happen with annoying regularity. Maybe in this toy example we ought to be implementing a separate method for quantities anyway, although the code would be identical I think.

@timholy
Copy link
Contributor Author

timholy commented Jul 30, 2016

I agree that such methods are likely to be problematic---when julia gets traits, it should be better because we might be able to say middle(x::Number::Tuple{ImplementsDiv,HasOrder}, ...) or something (basically the set of traits that might have motivated the author to pick Real). At least now it's possible to "specialize" them for such additional number types.

@ajkeller34
Copy link
Collaborator

ajkeller34 commented Aug 10, 2016

@timholy, are you able to review the branch "rewrite"? I've implemented a lot of changes and added some documentation. I haven't pushed this to master because I am hoping you can look it over and see what you think before forcing major changes on users. All existing tests pass. Hopefully we can work together to get something we're both happy with published and tagged. I dropped the use of convert to strip or add units although I'm still using it for other ill-advised purposes... maybe we should define unitconvert or something?

Note that you now need to call Unitful.defaults() after using Unitful. I'm not sure if this is the best approach, but in the old version, a user would need to make the package dirty to change their default units, which I didn't like.

edit: you can now do things like f(x::Length) = ... or f(x::Area) = ... which is kind of neat.

@ajkeller34
Copy link
Collaborator

I've just updated the "rewrite" branch again. I've adopted the strategy @dhoegh takes in EngUnits.jl to define default units, which solves the dirty package issue I was having. I've adopted his string macro for units now, e.g. u"kg*m/s^2" which is really just shorthand for Unitful.kg*Unitful.m/(Unitful.s)^2. Units, dimensions, fundamental constants, etc. are no longer exported, so it is up to the user to do kg = u"kg" if they want to bring kg into scope. This should address the first of the two concerns in #5, which I think is pretty important. Note the new dimension aliases are not exported either (f(x::Unitful.Length) = whatever).

Shortly after 0.5 is released I hope to publish and tag this, if only so that I am compelled to settle down with the breaking changes so that I start using this in my own projects in earnest. I will continue to tweak things in the mean time.

@dhoegh
Copy link

dhoegh commented Aug 15, 2016

@ajkeller34 I am glad my idea could be used. When I get time I will take it for a spin.

@ajkeller34
Copy link
Collaborator

@timholy Just a heads up that some Julia change has killed all of my array tests using 0.5.0-rc3 for the code in my rewrite branch. All was well on rc2. I wonder if it is related to any of the following:

JuliaLang/julia@5d2ac0d
JuliaLang/julia@4160a8a
JuliaLang/julia@b9d7579

I gave it a few hours of debugging but maybe it is better to debug once rc4 or final comes out. Strangely, array math will work or not work depending on if promote_op has been called explicitly in the REPL before the array math is attempted. Example:

Initial failure:

julia> using Unitful

julia> 5u"V" + [1u"V",2u"V"]
ERROR: TypeError: Quantity: in T, expected T<:Number, got Type{Any}
 in .+(::Unitful.Quantity{Int64,Unitful.Dimensions{(𝐈^-1,𝐋^2,𝐌,𝐓^-3)},Unitful.Units{(V,)}}, ::Array{Unitful.Quantity{Int64,Unitful.Dimensions{(𝐈^-1,𝐋^2,𝐌,𝐓^-3)},Unitful.Units{(V,)}},1}) at ./arraymath.jl:67
 in +(::Unitful.Quantity{Int64,Unitful.Dimensions{(𝐈^-1,𝐋^2,𝐌,𝐓^-3)},Unitful.Units{(V,)}}, ::Array{Unitful.Quantity{Int64,Unitful.Dimensions{(𝐈^-1,𝐋^2,𝐌,𝐓^-3)},Unitful.Units{(V,)}},1}) at ./arraymath.jl:95

Peculiar success:

julia> using Unitful
INFO: Recompiling stale cache file /Users/ajkeller/.julia/lib/v0.5/Unitful.ji for module Unitful.

julia> Base.promote_op(.+, typeof(5u"m"), typeof(1u"m"))
Unitful.Quantity{Int64,Unitful.Dimensions{(𝐋,)},Unitful.Units{(m,)}}

julia> 5u"V" + [1u"V",2u"V"] 
2-element Array{Unitful.Quantity{Int64,Unitful.Dimensions{(𝐈^-1,𝐋^2,𝐌,𝐓^-3)},Unitful.Units{(V,)}},1}:
 6 V
 7 V

Note that I didn't even have to use volts when I called promote_op. I'm willing to change my code if needed but this seems like a bug to me.

@timholy
Copy link
Contributor Author

timholy commented Sep 7, 2016

Sorry I've not had the time to check in on this. I just checked this on master, and it seems like you've figured it out?

@ajkeller34
Copy link
Collaborator

Thank you, I figured it out. I think it was from an overuse of generated functions that somehow resulted in different behavior between rc2 and rc3. I am using generated functions to do pre-computation for unit conversion, as well as to avoid type instabilities where I didn't see how to do so otherwise (sqrt and inv). It's possible I'm using them in a few places where they aren't really necessary.

The most recent code is now on the master branch of this package, and the documentation has been updated. I still welcome any feedback you have! I'm starting to try and use this package in earnest, so there's no better time than now to raise lingering concerns you have with it.

@timholy
Copy link
Contributor Author

timholy commented Sep 7, 2016

If I don't review it by the end of this week, ping me again. Got a few other items in the queue first, though.

@TotalVerb
Copy link

@ajkeller34 As requested, here are my two cents on whether unitful quantities should subtype Number.

Number is really an unfortunately vague choice of type name. The lack of definition in Base also does not help pinpoint what it means. So there can be no theoretical argument as to whether unitful quantities are algebraically Numbers, as there is no definition of Number.

Whether to do it, then, should be entirely a pragmatic decision. I see two things to consider:

  • One might like to be able to use functions in Base that are written generically, but dispatched on Number.
  • At the same time, one would prefer not to break any assumptions Base makes about Number.

My opinion is that Base has little generic code applicable beyond Number, and makes quite strong assumptions about it that unitful quantities cannot hope to satisfy.

I ran grep on ::Number in Julia's current base/ directory. There were many results, so I just took a look at the first few files (hopefully a sample of the consequences):

  • broadcast.jl: Number treated as scalar in broadcast; this is probably going to be obsolete after Generalize broadcast to handle tuples and scalars JuliaLang/julia#16986 is merged.
  • linalg.jl: scale! is restricted to Number. In-place scaling by a unitful quantity seems like a strange operation algebraically: it would necessarily change the units of the matrix, so how could it be in-place? Hence this is an argument for not subtyping Number.
  • sparsevector.jl/sparsematrix.jl: I don't have enough experience with sparse vectors to comment on the functions here.
  • array.jl: setindex! is specialized on Number. As far as I can tell, this is just an optimization, and should easily be able to be extended to any scalar in Base. That can probably be done after the broadcast changes.

My suspicion is that Base's numeric hierarchy is insufficient to properly handle quantities with physical units, and that some system of traits that describe algebraic properties (instead of vague categories like "Number") will be necessary. In the interim, I think it might be better not to subtype Number, and try to fix overspecialized methods in Base.

@TotalVerb
Copy link

TotalVerb commented Sep 10, 2016

Great job on this package, by the way. I have been monitoring its progress excitedly.

@andyferris
Copy link

andyferris commented Sep 10, 2016

To butt in here - I feel that I know I can do sin(x) if x is a Number but I can't take the sine of 1 metre. This argument gets a little muddled with degrees vs radians, etc.

Also numbers of the same type are usually closed under multiplication. Lots of code uses promote_type(...) not promote_op(*, ...).

@andyferris
Copy link

Also, I'm glad we're all agreeing that one() gives a multiplicative identity. The discussions about UnitRange I saw elsewhere were a little worrying.

I'm looking forward to using Unitful.jl. I had to implement some limited subset of ideas in Chrono.jl but really wished that stuff already existed.

I think traits would be pretty kick-ass in this package. For the moment, I'd think some dimensions-based traits would be a good idea (like Length or maybe a parameterized one) so that people can use them in their own thunks. If Julia ever adopts the kind of syntax @timholy discussed (see also Traitor.jl) they will be immediately incredibly useful.

@ajkeller34
Copy link
Collaborator

Not to beat a dead horse, but I don't think the meaning of UnitRange is actually specified anywhere in the Julia documentation. Same goes for Number, as pointed out by @TotalVerb above. It's difficult to satisfy everyone if there's no explicit contract to abide by. On the other hand, one(T) is defined to give the multiplicative identity for type T in the documentation. Uses contrary to that are obviously wrong. Although, calling this function one is kind of ambiguous in certain cases, since it could also refer to the "additive one" for a given type. Too bad multident sounds like a toothpaste brand.

I'm not sure if you saw, but you can already dispatch on Length quantities with this package as x::Unitful.Length. You can do it with quantities of any dimension, even "derived" ones that are some combination of the base SI dimensions: x::Unitful.Force, x::Unitful.Energy, etc. Or maybe I misunderstood what you meant about dimensions-based traits?

You bring up a good point regarding angles. Currently my package defines an angle dimension and rad and º units. I should not have defined an angle dimension: the circumference of a circle should be (2π radians)*(radius) which better have dimensions of length. Oops. Perhaps I can remove the angle dimension and let rad and º be dimensionless units, like I do with mV/V and similar combinations.

Regarding promote_type vs. promote_op, it seems like maybe these ideas are in flux in Julia master? I haven't had time to read all the relevant issues but I wouldn't be surprised if I have to redo some of the promotion code for Julia 0.6. Maybe some clarity on what a Number is would help people decide what assumptions they are allowed to make with promotion in their code.

Thanks to you both for your feedback. When I get a moment, maybe I'll try making a branch where quantities are not Numbers and see how that goes.

@andyferris
Copy link

Too bad multident sounds like a toothpaste brand.

I think this line of thinking is the bit that worried me. Surely having unit for the additive generator is much better than messing with the (more common?) multiplicative identity.

I'm not sure if you saw, but you can already dispatch on Length quantities with this package as x::Unitful.Length. You can do it with quantities of any dimension, even "derived" ones that are some combination of the base SI dimensions: x::Unitful.Force, x::Unitful.Energy, etc. Or maybe I misunderstood what you meant about dimensions-based traits?

No, sorry, I guess you are right - I haven't used it much at all yet. From following the above, I thought maybe it would be difficult to dispatch on both dimensions and number type simultaneously, but I'm probably wrong. Also, I wasn't sure if it would be easy (or desirable) for users to implement their own types with dimensions. And then you might consider that a vector is a Length if it's eltype is a Length, etc, etc. These are all probably non-problems.

You bring up a good point regarding angles. Currently my package defines an angle dimension and rad and º units. I should not have defined an angle dimension: the circumference of a circle should be (2π radians)*(radius) which better have dimensions of length. Oops. Perhaps I can remove the angle dimension and let rad and º be dimensionless units, like I do with mV/V and similar combinations.

That sounds like a good idea. It would be great if, e.g., sin could still dispatch on angle type, and I probably (??) want (3.14rad)::Number. Maybe all dimensionless (but unitful) quantities are a Number?

Regarding promote_type vs. promote_op, it seems like maybe these ideas are in flux in Julia master?

I'm thinking more-and-more that promote_op makes a lot of sense - e.g. it should remain consistent with inference. I guess I'll have to update some code in StaticArrays. The extra complication is unfortunate but it should be more correct.

@timholy
Copy link
Contributor Author

timholy commented Sep 12, 2016

I've now taken a gander over the docs, tests, and a bit at the code itself. I haven't used it other than just playing around, and if it's like any other code in the universe then some issues will doubtlessly pop up, but on first glance I didn't see anything that worried me.

I'll see if I can break some stuff perhaps contribute by adding @inferred to more of your tests. (I really like how in the docs you pointed out that (1m)^2 isn't inferrable, so I'm aware of the limitations for some things.)

Overall, I have to say: I'm basically blown away. This seems to be the units package of my dreams.

@ajkeller34
Copy link
Collaborator

Many thanks to @timholy for the kind words, and especially for the time spent looking over the code!

I'm inclined to leave things as is for now with regards to Number subtyping. I setup a branch available for all to see ("nonumber") that passes most tests after some tweaking, but I'm not convinced it is worth my effort to fix the remaining tests (all array math). I'd need to duplicate code that I'd otherwise get for free as defined in arraymath.jl. More troubling, should the corresponding code change in Base, the code in Unitful could get out of date. When there is an official party line regarding what a Number is and is not in Julia, I will make sure Unitful plays by the rules. I don't think this choice will affect most uses of the package, but I will review PRs if anyone is disappointed by this.

I'll stick to the plan of registering the package shortly after 0.5.0 is out.

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

No branches or pull requests

5 participants