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

Provide suggestions for unknown kwargs passed to blocks #4392

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

DanielVandH
Copy link
Contributor

@DanielVandH DanielVandH commented Sep 20, 2024

Description

Fixes #4391. There might be a better way to do what I'm doing here, but at least it's something to test with instead of my code snippets on that issue.

Adds a check to block constructors to see if the passed kwargs are all valid. Reuses the InvalidAttributeError code. Some examples:

julia> fig = Figure(); ax = Axis(fig[1, 1], badkwarg = 1, badkwarg2 = 2, xlabel = "x")
ERROR: Invalid attributes badkwarg and badkwarg2 for block type Axis.

The available block attributes for Axis are:

alignmode           leftspinevisible    subtitlevisible  topspinevisible   xlabelpadding      xminorticksvisible  xticklabelfont      xzoomkey          ylabelrotation      yminortickwidth  yticklabelpad       yzoomlock  
aspect              limits              tellheight       valign            xlabelrotation     xminortickwidth     xticklabelpad       xzoomlock         ylabelsize          ypankey          yticklabelrotation  zoombutton 
autolimitaspect     panbutton           tellwidth        width             xlabelsize         xpankey             xticklabelrotation  yautolimitmargin  ylabelvisible       ypanlock         yticklabelsize
backgroundcolor     rightspinecolor     title            xautolimitmargin  xlabelvisible      xpanlock            xticklabelsize      yaxisposition     yminorgridcolor     yrectzoom        yticklabelspace
bottomspinecolor    rightspinevisible   titlealign       xaxisposition     xminorgridcolor    xrectzoom           xticklabelspace     ygridcolor        yminorgridstyle     yreversed        yticklabelsvisible
bottomspinevisible  spinewidth          titlecolor       xgridcolor        xminorgridstyle    xreversed           xticklabelsvisible  ygridstyle        yminorgridvisible   yscale           yticks
dim1_conversion     subtitle            titlefont        xgridstyle        xminorgridvisible  xscale              xticks              ygridvisible      yminorgridwidth     ytickalign       yticksize
dim2_conversion     subtitlecolor       titlegap         xgridvisible      xminorgridwidth    xtickalign          xticksize           ygridwidth        yminortickalign     ytickcolor       yticksmirrored
flip_ylabel         subtitlefont        titlelineheight  xgridwidth        xminortickalign    xtickcolor          xticksmirrored      ylabel            yminortickcolor     ytickformat      yticksvisible
halign              subtitlegap         titlesize        xlabel            xminortickcolor    xtickformat         xticksvisible       ylabelcolor       yminorticks         yticklabelalign  ytickwidth
height              subtitlelineheight  titlevisible     xlabelcolor       xminorticks        xticklabelalign     xtickwidth          ylabelfont        yminorticksize      yticklabelcolor  ytrimspine
leftspinecolor      subtitlesize        topspinecolor    xlabelfont        xminorticksize     xticklabelcolor     xtrimspine          ylabelpadding     yminorticksvisible  yticklabelfont   yzoomkey


Stacktrace:
 [1] _check_remaining_kwargs(T::Type{Axis}, kwdict::Dict{Symbol, Any})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:298
 [2] _block(T::Type{…}, fig_or_scene::Figure, args::Vector{…}, kwdict::Dict{…}, bbox::Nothing; kwdict_complete::Bool)
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:316
 [3] _block
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:303 [inlined]
 [4] #_block#1490
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:259 [inlined]
 [5] _block(::Type{Axis}, ::GridPosition; kwargs::@Kwargs{badkwarg::Int64, badkwarg2::Int64, xlabel::String})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:253
 [6] _block
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:246 [inlined]
 [7] #_#1488
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:237 [inlined]
 [8] top-level scope
   @ REPL[25]:1
Some type information was truncated. Use `show(err)` to see complete types.

julia> lines([2.0], [2.0], axis = (xlabel="x", bad = 1, bad2 = [2]))
ERROR: Invalid attributes bad and bad2 for block type Axis.

The available block attributes for Axis are:

alignmode           leftspinevisible    subtitlevisible  topspinevisible   xlabelpadding      xminorticksvisible  xticklabelfont      xzoomkey          ylabelrotation      yminortickwidth  yticklabelpad       yzoomlock  
aspect              limits              tellheight       valign            xlabelrotation     xminortickwidth     xticklabelpad       xzoomlock         ylabelsize          ypankey          yticklabelrotation  zoombutton 
autolimitaspect     panbutton           tellwidth        width             xlabelsize         xpankey             xticklabelrotation  yautolimitmargin  ylabelvisible       ypanlock         yticklabelsize
backgroundcolor     rightspinecolor     title            xautolimitmargin  xlabelvisible      xpanlock            xticklabelsize      yaxisposition     yminorgridcolor     yrectzoom        yticklabelspace
bottomspinecolor    rightspinevisible   titlealign       xaxisposition     xminorgridcolor    xrectzoom           xticklabelspace     ygridcolor        yminorgridstyle     yreversed        yticklabelsvisible
bottomspinevisible  spinewidth          titlecolor       xgridcolor        xminorgridstyle    xreversed           xticklabelsvisible  ygridstyle        yminorgridvisible   yscale           yticks
dim1_conversion     subtitle            titlefont        xgridstyle        xminorgridvisible  xscale              xticks              ygridvisible      yminorgridwidth     ytickalign       yticksize
dim2_conversion     subtitlecolor       titlegap         xgridvisible      xminorgridwidth    xtickalign          xticksize           ygridwidth        yminortickalign     ytickcolor       yticksmirrored
flip_ylabel         subtitlefont        titlelineheight  xgridwidth        xminortickalign    xtickcolor          xticksmirrored      ylabel            yminortickcolor     ytickformat      yticksvisible
halign              subtitlegap         titlesize        xlabel            xminortickcolor    xtickformat         xticksvisible       ylabelcolor       yminorticks         yticklabelalign  ytickwidth
height              subtitlelineheight  titlevisible     xlabelcolor       xminorticks        xticklabelalign     xtickwidth          ylabelfont        yminorticksize      yticklabelcolor  ytrimspine
leftspinecolor      subtitlesize        topspinecolor    xlabelfont        xminorticksize     xticklabelcolor     xtrimspine          ylabelpadding     yminorticksvisible  yticklabelfont   yzoomkey


Stacktrace:
 [1] _check_remaining_kwargs(T::Type{Axis}, kwdict::Dict{Symbol, Any})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:298
 [2] _block(T::Type{…}, fig_or_scene::Figure, args::Vector{…}, kwdict::Dict{…}, bbox::Nothing; kwdict_complete::Bool)
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:316
 [3] _block(T::Type{Axis}, fig_or_scene::Figure, args::Vector{Any}, kwdict::Dict{Symbol, Any}, bbox::Nothing)
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:303
 [4] create_axis_for_plot(figure::Figure, plot::Lines{Tuple{Vector{Point{2, Float64}}}}, attributes::Dict{Symbol, Any})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\figureplotting.jl:107
 [5] create_axis_like(plot::Lines{Tuple{Vector{Point{2, Float64}}}}, attributes::Dict{Symbol, Any}, ::Nothing)
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\figureplotting.jl:199
 [6] _create_plot(::Function, ::Dict{Symbol, Any}, ::Vector{Float64}, ::Vararg{Vector{Float64}})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\figureplotting.jl:317
 [7] lines(::Vector{Float64}, ::Vararg{Vector{…}}; kw::@Kwargs{axis::@NamedTuple{}})
   @ MakieCore c:\Users\danjv\.julia\dev\Makie.jl\MakieCore\src\recipes.jl:449
 [8] top-level scope
   @ REPL[26]:1
Some type information was truncated. Use `show(err)` to see complete types.

julia> Label(fig[1,1], bad = 1)
ERROR: Invalid attribute bad for block type Label.

The available block attributes for Label are:

alignmode  color  font  fontsize  halign  height  justification  lineheight  padding  rotation  tellheight  tellwidth  text  valign  visible  width  word_wrap


Stacktrace:
 [1] _check_remaining_kwargs(T::Type{Label}, kwdict::Dict{Symbol, Any})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:298
 [2] _block(T::Type{…}, fig_or_scene::Figure, args::Vector{…}, kwdict::Dict{…}, bbox::Nothing; kwdict_complete::Bool)
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:316
 [3] _block
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:303 [inlined]
 [4] #_block#1490
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:259 [inlined]
 [5] _block(::Type{Label}, ::GridPosition; kwargs::@Kwargs{bad::Int64})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:253
 [6] _block
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:246 [inlined]
 [7] #_#1488
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:237 [inlined]
 [8] top-level scope
   @ REPL[27]:1
Some type information was truncated. Use `show(err)` to see complete types.

julia> Axis3(fig[1, 1], blah = "1")
ERROR: Invalid attribute blah for block type Axis3.

The available block attributes for Axis3 are:

alignmode        limits           titlegap          xlabel          xspinecolor_2    xticklabelsvisible  ygridcolor      ylabelvisible    yticklabelfont      zautolimitmargin  zlabelsize      zticklabelcolor
aspect           perspectiveness  titlesize         xlabelalign     xspinecolor_3    xticks              ygridvisible    yreversed        yticklabelpad       zgridcolor        zlabelvisible   zticklabelfont
azimuth          protrusions      titlevisible      xlabelcolor     xspinesvisible   xticksize           ygridwidth      yspinecolor_1    yticklabelsize      zgridvisible      zreversed       zticklabelpad
backgroundcolor  targetlimits     valign            xlabelfont      xspinewidth      xticksvisible       ylabel          yspinecolor_2    yticklabelsvisible  zgridwidth        zspinecolor_1   zticklabelsize
dim1_conversion  tellheight       viewmode          xlabeloffset    xtickcolor       xtickwidth          ylabelalign     yspinecolor_3    yticks              zlabel            zspinecolor_2   zticklabelsvisible
dim2_conversion  tellwidth        width             xlabelrotation  xtickformat      xypanelcolor        ylabelcolor     yspinesvisible   yticksize           zlabelalign       zspinecolor_3   zticks
dim3_conversion  title            xautolimitmargin  xlabelsize      xticklabelcolor  xypanelvisible      ylabelfont      yspinewidth      yticksvisible       zlabelcolor       zspinesvisible  zticksize
elevation        titlealign       xgridcolor        xlabelvisible   xticklabelfont   xzpanelcolor        ylabeloffset    ytickcolor       ytickwidth          zlabelfont        zspinewidth     zticksvisible
halign           titlecolor       xgridvisible      xreversed       xticklabelpad    xzpanelvisible      ylabelrotation  ytickformat      yzpanelcolor        zlabeloffset      ztickcolor      ztickwidth
height           titlefont        xgridwidth        xspinecolor_1   xticklabelsize   yautolimitmargin    ylabelsize      yticklabelcolor  yzpanelvisible      zlabelrotation    ztickformat


Stacktrace:
 [1] _check_remaining_kwargs(T::Type{Axis3}, kwdict::Dict{Symbol, Any})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:298
 [2] _block(T::Type{…}, fig_or_scene::Figure, args::Vector{…}, kwdict::Dict{…}, bbox::Nothing; kwdict_complete::Bool)
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:316
 [3] _block
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:303 [inlined]
 [4] #_block#1490
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:259 [inlined]
 [5] _block(::Type{Axis3}, ::GridPosition; kwargs::@Kwargs{blah::String})
   @ Makie c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:253
 [6] _block
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:246 [inlined]
 [7] #_#1488
   @ c:\Users\danjv\.julia\dev\Makie.jl\src\makielayout\blocks.jl:237 [inlined]
 [8] top-level scope
   @ REPL[28]:1
Some type information was truncated. Use `show(err)` to see complete types.

I wanted to get #4390 here too, but Figure isn't a block... the bad keyword arguments are just passed into Scene, e.g.

julia> Figure(bad=1).scene.theme[:bad]
Observable{Any}(1)

Not sure if that's a simple fix. I've never used themes. So, I won't address it here and keep it simple.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added unit tests for new algorithms, conversion methods, etc.

@asinghvi17
Copy link
Member

Thanks for the PR!

I don't think this qualifies as breaking if it's just changing the type of the error? Any situation that triggers this would have errored anyway.

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Sep 20, 2024

Right. I was thinking of the case where kwargs used to get silently ignored in recipes but that's not the case here.

@asinghvi17
Copy link
Member

 exception =
│    Invalid attribute entrygroups for block type Legend.
│    
│    The available block attributes for Legend are:
│    
│    alignmode        labeljustification  markerstrokecolor  rowgap       
│    backgroundcolor  labelsize           markerstrokewidth  tellheight   
│    bgcolor          labelvalign         nbanks             tellwidth    
│    colgap           linecolor           orientation        titlecolor   
│    framecolor       linecolormap        padding            titlefont    
│    framevisible     linecolorrange      patchcolor         titlegap     
│    framewidth       linepoints          patchlabelgap      titlehalign  
│    gridshalign      linestyle           patchsize          titleposition
│    gridsvalign      linewidth           patchstrokecolor   titlesize    
│    groupgap         margin              patchstrokewidth   titlevalign  
│    halign           marker              polycolor          titlevisible 
│    height           markercolor         polycolormap       valign       
│    label            markercolormap      polycolorrange     width        
│    labelcolor       markercolorrange    polypoints                      
│    labelfont        markerpoints        polystrokecolor                 
│    labelhalign      markersize          polystrokewidth  

hehe...from the docs build

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Sep 20, 2024

Legend seems weird. The entrygroups isn't an actual attribute, it's just a field defined in the struct defined in the @Block definition. But in the methods that are actually using it, it seems to get added directly in as an attribute afterwards.. what if I just do

function MakieCore.__valid_attributes(T::Type{<:Legend}) 
    atrs = _attribute_docs(Legend)
    atrs[:entrygroups] = ""
    return keys(atrs) 
end

unless someone can comment on what's going on with entrygroups here..

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Sep 20, 2024

There are still some issues with the tests with this approach. I am guessing my method of determining the valid attributes is just not great, e.g. pallete and entrygroups are apparently valid for Axis but are not found with my method.

@jkrumbiegel
Copy link
Member

I think given that there are only one or two exceptions to the rule that Blocks have no further kwargs than their attributes, and it's not sure if that's actually necessary, it would be worth refactoring that mechanism such that initialize never takes kwargs. To me the feedback mechanism for wrong usage is much more important than these edge cases. If they cannot be removed completely.

@DanielVandH
Copy link
Contributor Author

Should that refactoring be handled in this PR? Currently I'm just trying to give a bandaid fix to my current solution but it could be pretty simple to refine it later.

@jkrumbiegel
Copy link
Member

I haven't checked yet what your solution is, but maybe these kwarg uses can just be converted to attributes.

@DanielVandH
Copy link
Contributor Author

I'll try and get that implemented.

@DanielVandH
Copy link
Contributor Author

There are a bunch of annoying parts here that I don't understand when trying to convert the kwargs to just be attributes. I think it would be simpler for this PR to just leave it as is and then come back to this issue at another time?

@jkrumbiegel
Copy link
Member

Sure, so what are you doing with those keywords now?

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Sep 21, 2024

function MakieCore.__valid_attributes(T::Type{S}) where {S<:Block}
    attrs = _attribute_docs(T)
    # Some blocks have keyword arguments that are not attributes.
    # TODO: Refactor intiailize_block! to just not use kwargs?
    (S <: Axis || S <: PolarAxis) && (attrs[:palette] = "")
    S <: Legend && (attrs[:entrygroups] = "")
    S <: Menu && (attrs[:default] = "")
    S <: LScene && (attrs[:scenekw] = "")
    return keys(attrs)
end

They are just getting added into the available attributes (this function is only used in that error message)

@@ -288,6 +288,27 @@ function block_defaults(::Type{B}, attribute_kwargs::Dict, scene::Union{Nothing,
return attributes
end

MakieCore.__obj_name(::Type{<:Block}) = "block"
Copy link
Member

Choose a reason for hiding this comment

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

I quite dislike _funcname and I like __func_name even less :D
It seems like all of the __ functions could be easily replaced as well by just x isa Plot or attribute_names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about isa since I don't know what else there is besides x isa Plot and x isa Block. I could do x isa Plot though.

The __valid_attributes is because I didn't know if it made sense to just define attribute_names(::Type{<:Block}). Does it? It could replace __valid_atributes(::Type{<:Block}) below

Copy link
Member

Choose a reason for hiding this comment

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

attribute_names(::Type{<:Block}) sounds good to me, if @jkrumbiegel doesn't see a problem with it ;)
For the __obj_name, isn't that just string(block_or_plot_type)? I wouldn't mind it being a bit more verbose in favor of it being more correct and not introducing a new function for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried string initially but for some plot types like Triplot it shows all the parameters and takes up so much space. Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

Something like this?

x = typeof(plot_or_block)
string(x <: Plot ? Makie.plotsym(x) : x)

I'd just do it like this, but I'd also be happy to have a function for this, even as an interface without any _ ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I knew about plotsym. I'll look into all of this soon, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

Incorrect keyword arguments for axis argument don't show any suggestions
4 participants