-
Notifications
You must be signed in to change notification settings - Fork 472
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
Update plots before scraping #5321
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5321 +/- ##
==========================================
+ Coverage 96.20% 96.27% +0.06%
==========================================
Files 134 134
Lines 22718 22706 -12
==========================================
+ Hits 21856 21860 +4
+ Misses 862 846 -16 |
Thanks for taking the time to re-implement and scope down! Sorry for the messy process reverting your original PR. I'll review this ASAP (will be one evening this week) and make sure I:
|
Generally from a quick glance though this looks good, but I want to dive into the problem you're facing |
See the very zoomed in plots at the end here for the actual bug: https://mne.tools/mne-gui-addons/auto_examples/ieeg_locate.html#sphx-glr-auto-examples-ieeg-locate-py. The camera view is set using |
github-actions preview |
@ChristosT, since you're the most versed on the scraper at this time (being one of the only other people to modify it), would you please take a quick look at these changes? |
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.
Just some nitpicky comments, otherwise this looks good! Though, I'd like @ChristosT's feedback if available before merging since they are also modifying the scraper significantly in #4938
It looks good from my point of view. I merged this branch with my changes and built the documentation locally, as far as I checked everything looks good. We just need to be careful when merging because there is a (trivial) merge conflict in |
|
@alexrockhill Thank you for your flexibility in responding to our comments. It is a great contribution. |
Thanks for the reviews everyone |
Reimplements #5310.
@banesullivan I think these changes are much more modest and within the scope of the
pyvista
as opposed to thepyvistaqt
library. I'm glad we tested it withqtpy
compatibility so that we know this actually works in the original PR but we don't need to keep that all in the PR just to test one line so I think this should be okay now.