-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
@@ -3,3 +3,4 @@ julia 0.4 | |||
Reexport | |||
Plots | |||
LearnBase | |||
ValueHistories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to avoid adding dependencies, unless absolutely necessary. There are a few options... one of them is simply wrapping the include in a try block, so that if the user doesn't have the package it just doesn't include this functionality. Then we don't have to add it to the REQUIRE file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that I have seen that before. Do you have some example code?
Did you review whether these can be implemented as |
Wouldn't that require everything I pass to plot to be a subtype of The goal I have is to be able to use |
Let me think through this a little more, and I'll come back with what I would do. |
try it on the loss plots, which are a little more complicated as I would like to support something along the line of |
Ok I'll put together an IJulia notebook, and I might make some code changes On Tue, Nov 24, 2015 at 10:06 AM, Christof Stocker <notifications@github.com
|
Also you could make a small adjustment for the subplot case, which sets the titles instead of the labels if you have 1 loss per plot:
|
That seems cool! One question. How would I implement the functionality that allows I would guess: |
Sure that would work. And any variation of it, as long as the types aren't too generic. Just keep in mind... if you did something like:
then it would seriously break things since all float vectors would now pass through your recipe code. For custom types this shouldn't be an issue. |
Ok nice. I'll give it a shot. One concern though. Will |
That's up to you... the |
Can you post the code (or a gist) with a full example? Then I can help create the matching function. But generally, you can set
So you can get creative... |
so I am trying to change to this approach and I am getting a weird error function Plots._apply_recipe{I,V<:Real}(d::Dict, h::VectorUnivalueHistory{I,V}; kw...)
d[:marker] = get(d, :marker, (:ellipse, 1.5))
d[:legend] = get(d, :legend, false)
get(h) # Tuple{Array{Int64,1},Array{Float64,1}}
end
|
Are you sure you want to go this direction of having this custom way to define new plots? It feels really strange. overloading |
The
I'm pretty sure that it's a bad idea to overload Also remember that your average user won't be creating recipes (or at least, not right away) so it can be slightly less intuitive while saving tons of repeated (and error prone) code. |
So I gave it a shot. Let me know what you think. Any ideas how I could infer subplot(history)
|
Pull the latest dev branch, and you'll be able to do something like this:
Note: the If the user sets |
end | ||
end | ||
filtered | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be:
_filter_plotable_histories(h::DynMultivalueHistory) = filter((k,v) -> _is_plotable_history(v), h.storage)
I'm happy to merge what you have so far, and we can fix/fine-tune what we need to later. Also with regards to testing... I'm working on releasing a new package https://github.com/tbreloff/VisualRegressionTests.jl which will contain a generalized version of the regression testing that I'm doing in Plots currently. When that's ready to go we should build some examples which incorporate the plots we're building here, and run them as part of the travis testing. I'll get the framework setup... then you can add the tests for recipes that you add. |
A quick side note... thanks to this PR, I changed the default
Before it would error because of the missing |
Nice. Don't merge just yet. I would like to add the proposed changes and also do a little monkey testing. |
get!(d, :markershape, :ellipse)
get!(d, :markersize, 1.5) this works for |
Can you open an issue? Sounds like a bug, but I'm not at my computer.
|
I moved the files into sub-folders and implemented your suggestions. I will adapt the loss function plots in a separate PR. I'm ok with merging this if you are. concerning issue: yes sure, will do |
Add Plots for numeric ValueHistories
No description provided.