Skip to content
This repository has been archived by the owner on May 23, 2022. It is now read-only.

Properly display derived types of AbstractSet in REPL or IJulia #40

Open
ShuhuaGao opened this issue Aug 3, 2020 · 3 comments · May be fixed by #43
Open

Properly display derived types of AbstractSet in REPL or IJulia #40

ShuhuaGao opened this issue Aug 3, 2020 · 3 comments · May be fixed by #43

Comments

@ShuhuaGao
Copy link

ShuhuaGao commented Aug 3, 2020 •
edited
Loading

I encountered the issue when playing with Reinforce.jl, and the essential reason is in LearnBase.

Issue description:
When a concrete type in LearnBase derived from AbstractSet is displayed automatically in REPL or IJulia (i.e., with no semicolon at the end), an error will be caused like follows:

Error showing value of type LearnBase.DiscreteSet{Array{Int64,1}}:
ERROR: MethodError: no method matching iterate(::LearnBase.DiscreteSet{Array{Int64,1}})
...(a lot more, omitted here)

How to reproduce

julia> using LearnBase
julia> ds = LearnBase.DiscreteSet([1, 2, 3])

Note that, if you suppress the output with a semicolon and then print it manually with print(ds), then no error happens and the printed result is LearnBase.DiscreteSet{Array{Int64,1}}([1, 2, 3]).

Reason of the error
The reason is that when a variable is displayed automatically in REPL or IJulia, the display function is used. That is, if you print the output with display(ds), the same error is induced. It seems that, for subtypes of AbstractSet, the default display method tries to iterate over each element. However, there is no default implementation in Julia to iterate an AbstractSet. （see documentation)

Possible fix
Two obvious fixes are possible

  1. Add Base.iterate method for each related type in LearnBase.
    Example: if we dispatch Base.iterate for DiscreteSet by iterating DiscreteSet.items, the displayed output of the above ds is
LearnBase.DiscreteSet{Array{Int64,1}} with 3 elements:
  1
  2
  3

However, an iteration method may make little sense for LearnBase.IntervalSet.

  1. Support display by implementing the MIME show method for relevant types. (see documentation)
    Example:
Base.show(io::IO, ::MIME"text/plain", set::LearnBase.IntervalSet) = print(io, "$(typeof(set)):\n  ", "lo = $(set.lo)\n  ", "hi = $(set.hi)\n")

will display a LearnBase.IntervalSet(-1.0, 1.0) as

LearnBase.IntervalSet{Float64}:
  lo = -1.0
  hi = 1.0

My suggestion is that

  • Implement proper MIME show for all subtypes of AbstractSet pertaining to this issue.
  • For those subtypes that have iteration semantics (like DiscreteSet), implement also Base.iterate. Another benefit is that, with iteration support, those types can be used in a for loop naturally.

I can make a PR if you think the above suggestion is reasonable.
@juliohm
Copy link
Member

juliohm commented Sep 12, 2020

Can you please point out in the codebase where you think display is being defined? It is strange that you are getting a print issue when we don't define any show method in the codebase, as far as I remember.

@ShuhuaGao
Copy link
Author

ShuhuaGao commented Sep 13, 2020 •
edited
Loading

Can you please point out in the codebase where you think display is being defined? It is strange that you are getting a print issue when we don't define any show method in the codebase, as far as I remember.

Indeed no show method is defined. That is why the error is caused. Could you reproduce the error mentioned above?

Given an object x, Julia provides a default and proper implementation for show(x) and thus print(x). However, REPL and IJulia outputs an object with display(x) by default (i.e., with neither manual call of print etc. nor semicolon termination) For text output (which is true here), display(x) falls back to show(stdout, "text/plain", x) (see doc here).

Since no method show(stdout, "text/plain", x::LearnBase.IntervalSet) has been defined, the default dispatch of Julia seems to not work well and leads to the above errors. According to the error information, the default dispatch tries to call iterate on LearnBase.IntervalSet. The call of iterate is attributed to the subtyping of AbstractSet. In Julia codebase, there is (source)

function show(io::IO, ::MIME"text/plain", t::AbstractSet{T}) where T

The object t will be iterated inside the above function. Unfortunately, the Base.iterate method is not provided for LearnBase.DiscreteSet (and, e.g., LearnBase.IntervalSet). The key is that Julia provides no default implementation of Base.iterate for AbstractSet.

Overall, the issue can be easily fixed by providing show(stdout, "text/plain", x::LearnBase.DiscreteSet) or dispatching Base.iterate for DiscreteSet (and other similar types).

@juliohm
Copy link
Member

juliohm commented Sep 13, 2020

I can reproduce the issue, thanks for reporting. Do you mind submitting a PR with the show methods implemented? I am not following the AbstractSet concept in Base, nor these set types defined in LearnBase. It would also be nice to know where they are being used other than Reinforce.jl. We need to update LearnBase and continue with the clean up. I've been trying to find the time to work on it, but the GeoStats.jl stack is really consuming a lot of my time.

@ShuhuaGao ShuhuaGao linked a pull request Sep 13, 2020 that will close this issue
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Properly display derived types of AbstractSet in REPL or IJulia ShuhuaGao/LearnBase.jl
2 participants
@juliohm @ShuhuaGao