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

ones should call oneunit #23544

Closed
wants to merge 1 commit into from
Closed

Conversation

mdavezac
Copy link

@mdavezac mdavezac commented Sep 1, 2017

According to docstring, ones creates an array of T, hence it should call oneunit rather than one. See #16116 and #20268.

This PR requests makes the following call successful:

using Unitful
ones(typeof(1u"m"), 2)

Another option would be to add oneunits to Base?

mdavezac pushed a commit to mdavezac/Unitful.jl that referenced this pull request Sep 1, 2017
Overload for `ones` might not be necessary if JuliaLang/julia#23544 is
accepted.
@ararslan ararslan added the domain:arrays [a, r, r, a, y, s] label Sep 8, 2017
stevengj
stevengj previously approved these changes Sep 12, 2017
test/arrayops.jl Outdated
val::Int16
end
Base.one(::Type{OnesUsesOneunit}) = OnesUsesOneunit(1).val
Base.zero(::Type{OnesUsesOneunit}) = OnesUsesOneunit(0)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of defining your own type here, just use the Furlong type from tests/dimensionful.jl, which is there precisely to add tests for dimension bugs.

According to docstring, `ones` creates an array of `T`, hence it should
call `oneunit` rather than `one`. See JuliaLang#16116 and JuliaLang#20268.
@mdavezac
Copy link
Author

Thanks for the pointer to Furlong. The tests are now fixed.

@StefanKarpinski
Copy link
Sponsor Member

I'm not entirely sure about this. What's the algebraic / logical motivation for ones being unitful?

@TotalVerb
Copy link
Contributor

TotalVerb commented Sep 15, 2017

Is there any benefit to having ones(T, x, y) at all, instead of fill(one(T), x, y)?

Conducting a quick search reveals that most uses of ones should really be using fill to avoid intermediate allocations...

@mdavezac
Copy link
Author

To meones, zeros, similar, rand are all helper functions to get an array quickly. So, having ones behaved differently w.r.t type seems weird. From an algebraic point of view, for a vectorial space E over a field F, I would expect ones to return an element of E (as defined by the type T), not something composed of elements of F and creating an alternate vectorial space E' of it's own choosing. From F, I expect only scalars (for E), as given by one.

Obviously, there is scope for confusion between one and ones :(, so on that front, it might be better to have ones and oneunits. Or just oneunits, in a ground-zero world.

@TotalVerb
Copy link
Contributor

I'm suspicious of the oneunits(...) behavior because the oneunit is not algebraically meaningful. The matrix of ones is potentially meaningful as the identity for the Hadamard product, and in some other cases. What use case is there for a matrix of one units?

@mdavezac
Copy link
Author

In practice, creating a multidimensional array with units is useful in Physics for any number of cases.
So I would be inclined to have ones(Furlong{1, Int64}, 2, 3) give me a matrix of Furlongs.
However, if in other domains the expectation is that ones(Furlong{1, Int64}, 1, 2, 3) returns an array of Int64, then I'm happy to modify this PR that way.

Currently, it fails because we are creating an array of one type and filling it with another:

 julia> ones(Furlong{1, Int64}, 2, 3, 4)
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Furlong{1,Int64}
This may have arisen from a call to the constructor Furlong{1,Int64}(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] fill!(::Array{Furlong{1,Int64},3}, ::Int64) at ./multidimensional.jl:788
 [2] ones(::Type{T} where T, ::Int64, ::Int64, ::Int64, ::Vararg{Int64,N} where N) at ./array.jl:265

@TotalVerb
Copy link
Contributor

Could you give a specific case you have in mind?

@mdavezac
Copy link
Author

It could be a constant potential, for instance.

@TotalVerb
Copy link
Contributor

What advantage is there over fill(1V, dims...) which seems shorter and more flexible, then?

@mdavezac
Copy link
Author

Fair enough. But then why not just deprecate ones and zeros?

@TotalVerb
Copy link
Contributor

zeros might reasonably have a faster implementation than fill, by calling for example calloc. For almost all types oneunitswould have to overwrite memory anyway, so I don't think it's that useful as a separate function.

@mdavezac
Copy link
Author

Looks to me like the conclusion is this PR is not needed, and that ones should only be used for unitless quantities, if at all. Thanks for the review and comments.

@mdavezac mdavezac closed this Sep 17, 2017
@stevengj stevengj reopened this Sep 19, 2017
@stevengj stevengj closed this Sep 19, 2017
@fredrikekre fredrikekre mentioned this pull request Nov 5, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants