Update plots before scraping #5283

alexrockhill · 2023-12-05T21:51:42Z

alexrockhill · 2023-12-06T02:09:19Z

Can you check if the plotter is initialized before calling update? That would fix the errors:

FAILED tests/plotting/test_scraper.py::test_scraper[1] - RuntimeError: Render window interactor must be initialized before processing events.

Not sure how to do that if anyone has a second.

alexrockhill · 2023-12-06T04:52:28Z

I ran this locally and it fixes my issue.

codecov · 2023-12-06T05:22:04Z

Codecov Report

Merging #5283 (7b74324) into main (5b0d2ab) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5283   +/-   ##
=======================================
  Coverage   96.20%   96.20%           
=======================================
  Files         134      134           
  Lines       22712    22700   -12     
=======================================
- Hits        21850    21839   -11     
+ Misses        862      861    -1

tkoyama010 · 2023-12-06T06:44:53Z

Thanks for your contribution. To pass test coverage, we need a sample for adding a test. Please provide us with a sample that you have run locally.

alexrockhill · 2023-12-06T18:41:48Z

I ran the sample linked in the issue but that involves MNE and is far too large but the gist is to set the camera before rendering and make sure that has taken effect. I implemented a minimal version in tinypages. Should be good to go now.

tkoyama010 · 2023-12-06T20:17:38Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

… update

tkoyama010 · 2023-12-06T20:24:30Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

alexrockhill · 2023-12-06T20:28:49Z

Sorry about not figuring out the pre-commit protocol, got it working now and it's very nice.

I'm not sure if it's easiest to mark this to test it only on the CIs with the MNE dependencies. I'm not sure of a way to do that in this case though... pytest.mark I don't think would work. Maybe just some logic about trying and failing dependencies. I don't want it to be overly complicated and not look good though.

requirements_test.txt

tkoyama010 · 2023-12-06T20:42:07Z

Sorry about not figuring out the pre-commit protocol, got it working now and it's very nice.

Never mind. We are happy to show that our CI is excellent.

I'm not sure if it's easiest to mark this to test it only on the CIs with the MNE dependencies. I'm not sure of a way to do that in this case though... pytest.mark I don't think would work. Maybe just some logic about trying and failing dependencies. I don't want it to be overly complicated and not look good though.

~~I am not as familiar with this bug as you are, but this is a common problem in all environments in my mind. So I'm fine as is.~~ I got it what you say. This is the bug when Plotter has app. But still fine as is.

tkoyama010

Would you mind the output of the image to three pieces in order to keep the existing test intact?

tests/test_tinypages.py

tests/tinypages/plot_cone.py

tests/test_tinypages.py

tkoyama010

LGTM. Thanks!

alexrockhill · 2023-12-06T22:23:10Z

I moved the tests to test_scraper and where they actually had cover which was good because it actually needed to check not just that there was an app but also that it wasn't None.

tests/plotting/test_scraper.py

alexrockhill · 2023-12-07T01:05:55Z

I got down a bit of a rabbit hole trying to add coverage for the dynamic scraper but it looks like it doesn't have any coverage in general so maybe not the PR to do it in. I was close to adding it, just with plotters[n].show(interactive=False) there were 7 vtk objects that we not garbage collected. Might be something to look into as I guess that may cause memory leaks if you use the dynamic scraper?

Should be good to go after implementing the change you asked for with a new test function without branching logic.

tests/plotting/test_scraper.py

alexrockhill · 2023-12-07T17:37:16Z

Looks good to go by me

tkoyama010

LGTM. Let's make a test of DynamicScraper in the future.

alexrockhill · 2023-12-08T18:18:13Z

Thanks for the help and review @tkoyama010

banesullivan · 2023-12-09T03:12:41Z

.github/workflows/testing-and-deployment.yml

+      - name: Setup xvfb dependencies
+        run: |
+          git clone --depth=1 https://github.com/mne-tools/mne-python.git --branch main --single-branch
+          ./mne-python/tools/setup_xvfb.sh
+


Woah... why was this added here? This needs to be removed ASAP as this section of the tests is strictly non-graphics.

Further I want to discourage the use of custom scripts from external projects in our core CI routines. We should keep any configuration and set up limited to well-established GitHub Actions or scripts managed by PyVista.

Adding and executing arbitrary shell scripts is a major red flag and security risk albeit this is from a well-trusted downstream project.

That sounds reasonable. They were added for PyQtx.QWidgets.QApplication.process_events testing. Not sure there is a way to fake that for non-graphics tests, I just don't know there might be though. Otherwise since it was tested it could be no covered.

banesullivan · 2023-12-09T03:18:04Z

requirements_test.txt

+PyQt6!=6.6.1,<6.7.0
+PyQt6-Qt6!=6.6.1,<6.7.0
 pytest<7.5.0
 pytest-cov<4.2.0
 pytest-memprof<0.3.0
 pytest-xdist<3.4.0
 pytest_pyvista==0.1.8
+qtpy<2.5.0


We do not test Qt dependencies in PyVista. This was a part of the entire motivation for making PyVistaQt a separate repository

banesullivan · 2023-12-09T03:19:48Z

Apologies @alexrockhill and @tkoyama010 but these changes need to be reverted as they are too impactful and need to undergo a more thorough review

This reverts commit f96feb4.

banesullivan · 2023-12-09T03:59:07Z

pyvista/plotting/utilities/sphinx_gallery.py

+    def __repr__(self):
+        """Return a stable representation of the class instance."""
+        return f"<{type(self).__name__} object>"
+


@alexrockhill, why was this needed? We do not intend for this class to be used or displayed to users as far as I understand?

Asking because I'm going to see if I can re-land this PR with less impact

It was added when I was trying to add coverage of the dynamic scraper but then not removed when adding that coverage exposed a bug where the dynamic scraper failed to close plots and so that was punted to a future PR.

try update call

d5ba83e

tkoyama010 added the bug Uh-oh! Something isn't working as expected. label Dec 6, 2023

figured it out

f93ad14

fix failure

1ebe8cb

alexrockhill added 4 commits December 6, 2023 10:13

add test

444c87f

isort

2de61ec

Merge branch 'main' into update

dfad6ed

test changed not exact checksum

2ee4c09

alexrockhill added 4 commits December 6, 2023 11:48

hit new lines

ceb9116

isort

e876082

add test dep

65208f7

req in wrong place

84faff6

pre-commit-ci bot and others added 3 commits December 6, 2023 20:19

[pre-commit.ci] auto fixes from pre-commit.com hooks

c2a6efe

for more information, see https://pre-commit.ci

add bindings

63cb55c

Merge branch 'update' of https://github.com/alexrockhill/pyvista into…

e161c7a

… update

[pre-commit.ci] auto fixes from pre-commit.com hooks

a514b1a

for more information, see https://pre-commit.ci

tkoyama010 reviewed Dec 6, 2023

View reviewed changes

requirements_test.txt Outdated Show resolved Hide resolved

requirements_test.txt Outdated Show resolved Hide resolved

Apply suggestions from code review

9070a0e

alexrockhill added 2 commits December 6, 2023 13:05

configure dependencies

9111266

oops bad formatting

f4a003d

tkoyama010 requested changes Dec 6, 2023

View reviewed changes

tests/test_tinypages.py Outdated Show resolved Hide resolved

tests/tinypages/plot_cone.py Outdated Show resolved Hide resolved

tests/test_tinypages.py Outdated Show resolved Hide resolved

tkoyama010 and others added 3 commits December 7, 2023 06:31

Update tests/tinypages/plot_cone.py

f6b6109

Update tests/test_tinypages.py

97679cf

fix numbering

b15ef1b

tkoyama010 previously approved these changes Dec 6, 2023

View reviewed changes

add cover

a829fee

alexrockhill dismissed tkoyama010’s stale review via a829fee December 6, 2023 22:21

alexrockhill force-pushed the update branch from f814201 to a829fee Compare December 6, 2023 22:21

alexrockhill added 2 commits December 6, 2023 14:24

shoot forgot precommit

659f3c1

still no cover on dynamic scraper, refactor

e0a550e

tkoyama010 requested changes Dec 6, 2023

View reviewed changes

tests/plotting/test_scraper.py Outdated Show resolved Hide resolved

alexrockhill added 2 commits December 6, 2023 17:02

new function

5cb7ae4

forgot no cover

7959a11

tkoyama010 reviewed Dec 7, 2023

View reviewed changes

tests/plotting/test_scraper.py Outdated Show resolved Hide resolved

tests/plotting/test_scraper.py Outdated Show resolved Hide resolved

tkoyama010 and others added 3 commits December 7, 2023 10:54

Apply suggestions from code review

789d628

Merge branch 'main' of https://github.com/pyvista/pyvista into update

8110f17

fix coverage

7b74324

tkoyama010 approved these changes Dec 7, 2023

View reviewed changes

tkoyama010 merged commit f96feb4 into pyvista:main Dec 8, 2023
25 checks passed

alexrockhill deleted the update branch December 8, 2023 21:37

banesullivan reviewed Dec 9, 2023

View reviewed changes

banesullivan added a commit that referenced this pull request Dec 9, 2023

Revert "Update plots before scraping (#5283)"

cb9d0d3

This reverts commit f96feb4.

banesullivan mentioned this pull request Dec 9, 2023

Revert "Update plots before scraping" #5310

Merged

banesullivan added a commit that referenced this pull request Dec 9, 2023

Revert "Update plots before scraping (#5283)" (#5310)

f581a23

This reverts commit f96feb4.

banesullivan reviewed Dec 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update plots before scraping #5283

Update plots before scraping #5283

alexrockhill commented Dec 5, 2023

alexrockhill commented Dec 6, 2023

alexrockhill commented Dec 6, 2023

codecov bot commented Dec 6, 2023 •

edited

tkoyama010 commented Dec 6, 2023 •

edited

alexrockhill commented Dec 6, 2023 •

edited

tkoyama010 commented Dec 6, 2023

tkoyama010 commented Dec 6, 2023

alexrockhill commented Dec 6, 2023

tkoyama010 commented Dec 6, 2023 •

edited

tkoyama010 left a comment

tkoyama010 left a comment

alexrockhill commented Dec 6, 2023

alexrockhill commented Dec 7, 2023

alexrockhill commented Dec 7, 2023

tkoyama010 left a comment •

edited

alexrockhill commented Dec 8, 2023

banesullivan Dec 9, 2023

banesullivan Dec 9, 2023

alexrockhill Dec 9, 2023

banesullivan Dec 9, 2023

banesullivan commented Dec 9, 2023

banesullivan Dec 9, 2023

alexrockhill Dec 9, 2023

Update plots before scraping #5283

Update plots before scraping #5283

Conversation

alexrockhill commented Dec 5, 2023

alexrockhill commented Dec 6, 2023

alexrockhill commented Dec 6, 2023

codecov bot commented Dec 6, 2023 • edited

Codecov Report

tkoyama010 commented Dec 6, 2023 • edited

alexrockhill commented Dec 6, 2023 • edited

tkoyama010 commented Dec 6, 2023

tkoyama010 commented Dec 6, 2023

alexrockhill commented Dec 6, 2023

tkoyama010 commented Dec 6, 2023 • edited

tkoyama010 left a comment

Choose a reason for hiding this comment

tkoyama010 left a comment

Choose a reason for hiding this comment

alexrockhill commented Dec 6, 2023

alexrockhill commented Dec 7, 2023

alexrockhill commented Dec 7, 2023

tkoyama010 left a comment • edited

Choose a reason for hiding this comment

alexrockhill commented Dec 8, 2023

banesullivan Dec 9, 2023

Choose a reason for hiding this comment

banesullivan Dec 9, 2023

Choose a reason for hiding this comment

alexrockhill Dec 9, 2023

Choose a reason for hiding this comment

banesullivan Dec 9, 2023

Choose a reason for hiding this comment

banesullivan commented Dec 9, 2023

banesullivan Dec 9, 2023

Choose a reason for hiding this comment

alexrockhill Dec 9, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2023 •

edited

tkoyama010 commented Dec 6, 2023 •

edited

alexrockhill commented Dec 6, 2023 •

edited

tkoyama010 commented Dec 6, 2023 •

edited

tkoyama010 left a comment •

edited