-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adds demo for plotting sensors #79
Conversation
Looks good @caiw, but there are a couple of issues:
Could I leave with you to fix both of these? |
Oops yes I'll take a look |
|
I don't know. Second is the SI unit, but milliseconds is what we use day-to-day, and it always takes me a moment or so to mentally translate "0.045" into 45 milliseconds. I vote milliseconds. |
[2. is now fixed] |
Yeah I could go either way. I think we should definitely show milliseconds on x-axes, but I can also see an argument for using the SI unit in the datastructure. What does MNE do? |
One argument in favour of seconds is that all the .nkg hexel files we currently have use seconds. |
Anyway the source of bug (1.) is the different time units on the sensor and hexel .nkg files, not in the code (although manually setting the x-ticks is something we'll have to address eventually when using the next dataset). I suggest we wait until we've decided on a standard time unit and then get the sample datasets all to use that unit and then verify the plots look good before we merge this in. |
Thanks for your reviews @neukym! Catching my bugs :) |
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.
Done!
The system works! Merging now. |
Oh you've merged :-) |
Thanks! |
Starts fixing #78