Skip to content

Conversation

@likanzhan
Copy link
Contributor

No description provided.

@Tokazama
Copy link
Member

Looks like a good place to start. We should update the project.toml file if we add dependencies

Copy link

@palday palday left a comment

Choose a reason for hiding this comment

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

Looks great! I've got some comments to help transition it from "user code" to "package code"

Project.toml Outdated
version = "0.1.0"

[deps]
GLMakie = "e9467ef8-e4e7-5192-8a1a-b1aee30e663a"
Copy link

Choose a reason for hiding this comment

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

only depend on Makie for top-level package stuff and let the user load their preferred backend

plot_topography(PlotTopography.my_channel, rand(59))
```
"""
function plot_topography(channelList::Vector{String}, channelValues::Vector{Number}; gridSize = 1000)
Copy link

Choose a reason for hiding this comment

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

This method should probably be made internal (_plot_topography) and instead plot_topography should take a Tables.jl table.

(I'll help do this when I get a few minutes)


# Extend data beyond the boundary, see:
# https://www.mathworks.com/matlabcentral/fileexchange/72729-topographic-eeg-meg-plot
extendData = append_nearest_values(channelXs, channelYs, channelValues)
Copy link

Choose a reason for hiding this comment

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

original license here appears to be BSD-3, may need to add more license-related documentation at the method definition

end


"""
Copy link

Choose a reason for hiding this comment

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

should probably convert these constants to Tables.jl table, where each row is channel name + coordinates

Copy link
Member

@Tokazama Tokazama Jan 18, 2022

Choose a reason for hiding this comment

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

Or even just a NamedTuple. It might make sense to have a formal ElectrodeTemplate type at some point that is a table.

EDIT:
Actually, it would probably be good to have something like ROITable with optionally 2d or 3d coordinates. We can then add networks on top of that in the future, treating it like plotted graph. But that level of detail may be outside the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I stored it as a named tuple.

	modified:   Project.toml
	modified:   src/NeuroPlots.jl
@Tokazama
Copy link
Member

Thanks for this, @likanzhan. You're awesome!

@palday, since you have experience with plotting packages, what's your advice on testing here?

@palday
Copy link

palday commented Jan 22, 2022

@likanzhan and @Tokazama

The "best" way is to use something like Percy that does visual testing, so that you know if your plots change. But that takes a bit of effort and potentially a paid account, and I haven't done for my plotting packages yet, so I can't help do it quickly.

The next best way is a combination of two things:

  1. writing out the plots (e.g. with CairoMakie.save) to some temp dir during the normal tests stage just to make sure the code runs and doesn't error. You have to write the figure out to file because a lot doesn't happen until the figure is actually drawn.
  2. Having good docs where a few sample plots are displayed, so that you can also do quick visual checks on new plots to see if they look like you expect (and to see if some figures fail to render).

The combination of these two things are generally enough to get an impression about the state of the package and detect e.g. if a Makie version bump breaks anything.

@Tokazama
Copy link
Member

That's super helpful. Thank you.

I'm going to go ahead and merge this so we can incrementally build on this.

@Tokazama Tokazama merged commit f712797 into JuliaNeuroscience:master Jan 22, 2022
@jrevels
Copy link

jrevels commented Feb 23, 2022

The "best" way is to use something like Percy that does visual testing, so that you know if your plots change. But that takes a bit of effort and potentially a paid account, and I haven't done for my plotting packages yet, so I can't help do it quickly.

(cc @SimonDanisch who has been interested in developing an OSS/Julia-developer-friendly alternative to Percy for Makie + Julia ecosystem :) good to know there are use cases here)

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

Successfully merging this pull request may close these issues.

4 participants