Erpgrid #92
Erpgrid #92
Conversation
…ox/UnfoldMakie.jl into erpPlotDocumentation
|
Could you please look what's happened here - here was a huge merge conflict, I resolved it as i could, but not sure that it was correct (I didn't know about bunch of changes) |
|
everything until "small test" was mine |
src/plot_erpgrid.jl
Outdated
| f::Union{GridPosition,Figure}, | ||
| plotData::Matrix{<:Real}, | ||
| pos; | ||
| chanNum = 30, |
There was a problem hiding this comment.
why do we need chanNum? we can always do size(plotData,1)
There was a problem hiding this comment.
if you want to restrict the channels to be plotted, you'd rather expect a range/vector of channels to be used, else you can only restrict up to chanNum. I would remove this functionality and let the user subset channels / positions themselves
|
|
||
| function plot_erpgrid!( | ||
| f::Union{GridPosition,Figure}, | ||
| plotData::Matrix{<:Real}, |
There was a problem hiding this comment.
some functions work right now on dataframes, some on Matrix.
We should unify this. Unfold.jl by default returns DataFrames (via coeftable) which are basically linearized Matrices (e.g. plotData[:]). Not 100% sure how to best handle it, but it is always annoying to need to convert to the other format :)
| plot_erpgrid!(Figure(), plotData, pos; kwargs...) | ||
|
|
||
| function plot_erpgrid!( | ||
| f::Union{GridPosition,Figure}, |
There was a problem hiding this comment.
this is outside of this PR, but a new one:
Here the method signatures are defined, and we are not defining them correctly ^^
https://docs.makie.org/stable/explanations/plot_method_signatures/
There was a problem hiding this comment.
what do you mean exactly?
There was a problem hiding this comment.
I think plot_erpgrid(GridPosition ...) is what Makie writes in the docs should be defined, and plot_erpgrid!(axis) - so modifying only if you omdify the axis.
also returntype is very different for us, e.g. docs say:
The mutating methods always just return a plot object. If no figure is passed, the current_figure() is used, if no axis or scene is given, the current_axis() is used.
| pos = hcat([[p[1], p[2]] for p in pos]...) | ||
|
|
||
| pos = pos[:, 1:chanNum] | ||
| minmaxrange = (maximum(pos, dims = 2) - minimum(pos, dims = 2)) |
There was a problem hiding this comment.
we should move this standardization to an extra function. We need it in other places as well
| # todo: 0.1 should go into plot config | ||
| ax = Axis( | ||
| f[1, 1], | ||
| width = Relative(0.2), |
There was a problem hiding this comment.
the 0.2 should be a configurable quantitiy in the "visual" category
| if drawLabels | ||
| text!( | ||
| ax, | ||
| rel_zeropoint + 0.1, |
There was a problem hiding this comment.
the distance of the label should be configurable imho - in general all of these settings should be configurable, we can achieve this by either using a PlotConfig category, or by an argument::NamedTuple and then merging our defaults wiht the provided defaults
| push!(axlist, ax) | ||
| end | ||
| # todo: make optional + be able to specify the linewidth + color | ||
| hlines!.(axlist, Ref([0.0]), color = :gray, linewidth = 0.5) |
There was a problem hiding this comment.
same as above, but you already have a todo there :)
| hidedecorations!.(axlist) | ||
| hidespines!.(axlist) | ||
|
|
||
| f |
There was a problem hiding this comment.
maybe return not only f, but also the axlist?
There was a problem hiding this comment.
because we do the same in eeg_topolotseries - or whatever the function is called
There was a problem hiding this comment.
maybe we should look at all input + output-types and check what makes sense. a FigureAxisPlot object is on the table as well, but related to the mutating/non-mutating discussion
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #92 +/- ##
==========================================
- Coverage 69.44% 67.27% -2.18%
==========================================
Files 17 18 +1
Lines 635 660 +25
==========================================
+ Hits 441 444 +3
- Misses 194 216 +22
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
|
okay let's include this changes for now |
No description provided.