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

make order irrelevant to ambiguous method warnings #270

Closed
JeffBezanson opened this issue Dec 1, 2011 · 11 comments
Closed

make order irrelevant to ambiguous method warnings #270

JeffBezanson opened this issue Dec 1, 2011 · 11 comments
Assignees

Comments

@JeffBezanson
Copy link
Member

It would be nice to be smarter about giving these warnings, so that if the needed method is defined, say, before the end of a file, no warning is printed.

@vtjnash
Copy link
Member

vtjnash commented Feb 1, 2013

would it be reasonable to make the warnings print when a module is finished loading (with a special case for Main that continues with the current behavior)? Anyways, I don't see this as release blocking.

@StefanKarpinski
Copy link
Member

Another option is making it parse-unit-based. So warn at the end of file for file input or per input expression in the repl.

@timholy
Copy link
Member

timholy commented Jun 2, 2013

Now I'm beginning to wonder whether these warnings are the right way to go. I just loaded two packages, Images and Calendar, and was encouraged to define methods for doing arithmetic on Images of dates:

Warning: New definition 
    +(AbstractArray{T1<:Union(AbstractCalendarDuration,CalendarTime),N},AbstractArray{T2<:Union(AbstractCalendarDuration,CalendarTime),N}) at operators.jl:228
is ambiguous with 
    +(AbstractImageDirect{T,N},AbstractArray{T,N}) at /home/tim/.julia/Images/src/algorithms.jl:102.
Make sure 
    +(AbstractImageDirect{T1<:Union(AbstractCalendarDuration,CalendarTime),N},AbstractArray{T2<:Union(AbstractCalendarDuration,CalendarTime),N})
is defined first.

and so on.

While it's interesting to consider, I don't currently intend to define an image where each pixel is a CalendarTime. So in this case, this warning isn't terribly helpful.

I guess the problem is that, as the number of packages grows, it gets to be hard to avoid these warnings because one can't anticipate the various combinations of packages that will be used.

@diegozea
Copy link
Contributor

diegozea commented Jun 2, 2013

Related: #3178

@JeffBezanson
Copy link
Member Author

The warnings are always telling you something useful, even if adding crazy new methods is not the best fix. In fact the warning should also say "If the suggested new method seems crazy, then one of the other methods should probably be deleted."

Packages should really not define things like +(Array{Foo}, Array{Bar}), because + for arrays is already defined generically. And when others (like yourself) define other kinds of arrays, conflicts are then inevitable.

@vtjnash
Copy link
Member

vtjnash commented Jun 2, 2013

My initial thought was that perhaps a module should have a keyword to turn off all ambiguity warnings within that module (and make them happen at runtime when the user attempts to do something like images with CalendarTime pixels).

However, in this case, it's not immediately clear to me why an AbstractImage is a type of AbstractArray. While an image can be represented as an array and in many cases converted to an array, I think that is a rather different thing from saying that an image is a type of array.

@JeffBezanson
Copy link
Member Author

Maybe, but I say overriding +(AbstractArray, AbstractArray) for a certain element type is the worse infraction here. There is something intrinsically sketchy about that, while there is nothing intrinsically sketchy about defining a new type of AbstractArray.

@JeffBezanson
Copy link
Member Author

Also, I suspect our vectorize macros should only generate definitions for Array (since they only construct Arrays) and not AbstractArray.

@timholy
Copy link
Member

timholy commented May 5, 2016

Wow, this is the 11th-oldest open issue. Fun!

@timholy timholy closed this as completed May 5, 2016
@StefanKarpinski
Copy link
Member

Thanks, @timholy – I actually strongly believe that this will improve the "feel of maturity" of Julia considerably. All those warnings when loading packages, even if they were harmless did not look good or give a warm and fuzzy feeling.

@timholy
Copy link
Member

timholy commented May 5, 2016

julia-0.5 just feels SO solid. Aside from having some amazing new features and fewer performance gotchas than ever, I am grateful on a nearly daily basis for the work ferreting out GC/dispatch/etc problems and things like the work done improving the line numbers in backtraces. It really adds up to a great experience.

LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Oct 11, 2021
* Add Base.similar, merge and merge! for Histogram

* Use zero(h::Histogram) instead of similar(h::Histogram)

Suggested by @andreasnoack.

* Improve tests for zero and merge for Histogram
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

6 participants