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

Handle Dates-array arithmetic with promote_op #12370

Merged
merged 1 commit into from
Jul 30, 2015
Merged

Handle Dates-array arithmetic with promote_op #12370

merged 1 commit into from
Jul 30, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 29, 2015

This fixes the ambiguity warnings triggered by #12115. It also introduces a new abstract type, AbstractScalar and makes use of it in defining operations.

CC @GordStephen, @quinnj.

@tkelman
Copy link
Contributor

tkelman commented Jul 29, 2015

Can you describe briefly what the difference is between Number and AbstractScalar ? Which of Number's behaviors are not going to be defined by default for AbstractScalar ? Would any promotion (or other) documentation need to be adjusted for this?

(hopefully I'm not the only one who's going to have this question)

@@ -1,6 +1,6 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license

abstract AbstractTime
abstract AbstractTime <: AbstractScalar
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I guess this is ok. AbstractTime was kind of a catch all for all the Dates types, but this should be ok. The piece that may not make as much is the parsing Slot types, but those probably need to be reconfigured anyway.

@timholy
Copy link
Member Author

timholy commented Jul 29, 2015

Basically, the idea is that if someone was reluctant to call their type a Number, we should have some notion of AbstractScalar. So this is perhaps more of a question for @quinnj about why it isn't a Number.

@andreasnoack
Copy link
Member

I've actually wanted something like an AbstractScalar or NonArray for a while. Right now we have this definition for transpose which I'd really like to have narrowed down to something like NonArray/AbstractScalar

@timholy
Copy link
Member Author

timholy commented Jul 29, 2015

I should add that another approach would be to use traits and introduce isscalar.

@tkelman
Copy link
Contributor

tkelman commented Jul 29, 2015

This question comes up a lot, a few times at JuliaCon for example, people often want to have container types that are conceptually number-like but aren't necessarily compatible with every single thing that we define in base for Number. Maybe one more level of granularity in the abstract type hierarchy wouldn't hurt, but the real solution to the "which behaviors do you want" problem is out of scope here and will probably be something traits-like.

@timholy
Copy link
Member Author

timholy commented Jul 29, 2015

If we want to define AbstractTime <: Number then I'd be happy indeed to drop the first commit.

@lindahua
Copy link
Contributor

scalar (in math context) is mainly used to refer to the coefficients in linear algebra (see https://en.wikipedia.org/wiki/Scalar_(mathematics)). In this sense, it is more specialized (instead of more general) than number.

What about call it Atom, which indicates that the instance should be considered as a whole, instead of being a collection of ingredients.

@lindahua
Copy link
Contributor

In the mean time, for symmetry, we may also consider introducing something like AbstractContainer.

@mbauman
Copy link
Member

mbauman commented Jul 29, 2015

Some (brief) prior discussion here: #9270 (comment), which also mentions Atom. I feel like there's been other issues discussing this idea, but I can't find them now. If we do this, it should probably be exported like the other abstract types.

I don't like how this moves arithmetic from Number to AbstractScalar. I think arithmetic is inherently a number-y thing to do. While time Periods could be Numbers, I don't think Date should be.

Edit: Hm, looking closer I realized that it only defines mixed array/AbstractScalar arithmetic. I suppose that makes sense. Never mind me.

@timholy
Copy link
Member Author

timholy commented Jul 29, 2015

Thanks for the excellent discussion.

I'm tempted to rework this using traits. Methods like *(A::AbstractArray, B::Number) = A .* B would be redefined as

abstract IsContainerTrait
immutable AbstractContainer <: IsContainerTrait end
immutable AbstractAtom <: IsContainerTrait end

iscontainertrait(::AbstractArray) = AbstractContainer()
iscontainertrait(::Number) = AbstractAtom()
iscontainertrait(::AbstractTime) = AbstractAtom()

@inline *(A, B) = *((A, iscontainertrait(A)), (B, iscontainertrait(B)))
function *{AT,BT}(Atrait::Tuple{AT,AbstractContainer}, Btrait::Tuple{BT,AbstractAtom})
    ....
end

However, I worry about adding yet another system (even for internal use) this late in 0.4. At this point, I wonder if the best course would be to revert #12115 and have people who need this functionality get it via a package. See #12115 (comment). Thoughts? Especially @quinnj, who hasn't chimed in about that yet.

@mbauman
Copy link
Member

mbauman commented Jul 29, 2015

It seems like we really want to define *(::AbstractArray, ::Not{AbstractArray}), with some magical Not{} that's the opposite of Union{}.

@StefanKarpinski
Copy link
Member

It seems like we really want to define *(::AbstractArray, ::Not{AbstractArray}), with some magical Not{} that's the opposite of Union{}.

There goes decidability of subtyping.

@jakebolewski
Copy link
Member

You can do subtyping with negation types and have it be decidable.

@StefanKarpinski
Copy link
Member

Sure, but is Julia's subtyping decidable if you add negation types? I'm not sure, but I doubt it.

@timholy
Copy link
Member Author

timholy commented Jul 29, 2015

OK, I think I found a minimalistic way to do this. When I looked more carefully, the Array/Scalar operations were not really the core source of the problem.

@@ -58,6 +58,9 @@ call(::DotMulFun, x, y) = x .* y
immutable RDivFun <: Func{2} end
call(::RDivFun, x, y) = x / y

immutable DotRDivFun <: Func{2} end
call(::RDivFun, x, y) = x ./ y
Copy link
Member

Choose a reason for hiding this comment

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

call(::DotRDivFun?

Copy link
Member Author

Choose a reason for hiding this comment

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

'Doh! Thanks.

This avoids deprecation warnings with packages like Images and DataArrays
(hopefully others too). The key point seems to be to avoid specializing
on the element type while being generic about the container.
@tkelman
Copy link
Contributor

tkelman commented Jul 30, 2015

Yeah, this looks much simpler and uncontroversial now.

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.

8 participants