Skip to content

Conversation

@AlejandroFernandezLuces
Copy link
Contributor

@AlejandroFernandezLuces AlejandroFernandezLuces commented Apr 1, 2025

Description

Add a separate optional target from graphics installation, as required in ansys/pyansys#902

Checklist

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added dependencies maintenance General maintenance of the repo (libraries, cicd, etc) bug Issue, problem or error in PyMAPDL labels Apr 1, 2025
@codecov
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 89.09091% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.29%. Comparing base (e0dd5d6) to head (5f017d8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3820      +/-   ##
==========================================
- Coverage   88.36%   88.29%   -0.08%     
==========================================
  Files         187      187              
  Lines       14761    14819      +58     
==========================================
+ Hits        13044    13084      +40     
- Misses       1717     1735      +18     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@germa89 germa89 changed the title fix: Optional viz dependency fix: Optional graphics dependency Apr 1, 2025
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions github-actions bot added the documentation Documentation related (improving, adding, etc) label Apr 2, 2025
@AlejandroFernandezLuces
Copy link
Contributor Author

Before going forward and modify the examples, do you agree with this approach for backend selection @germa89 @RobPasMue ? User facing API would change like this:

    >>> from ansys.mapdl.core import GraphicsBackend
    >>> mapdl.aplot(graphics_backend=GraphicsBackend.MATPLOTLIB)
    >>> mapdl.lplot(graphics_backend=GraphicsBackend.MATPLOTLIB)
    >>> mapdl.kplot(graphics_backend=GraphicsBackend.MATPLOTLIB)

@germa89
Copy link
Collaborator

germa89 commented Apr 7, 2025

@AlejandroFernandezLuces I did figure out what was the problem with the tests. There was a few:

  1. One of the test was changing the default state of the MAPDL class to GraphicsBackend.MAPDL. That returns None when plotting (even if using return_plotter=True), hence the corresponding assertion errors. I did fix that test and add a check on the run_before_and_after function.

    eed1822 (Highly recommended to use patch!!
    ec45e05

  2. In PyMAPDL we test that some docstrings include certain things, which the recent changes they wasnt. 568fb42

  3. Also, wrong imports in here: 0963041

  4. Additionally, the documentation requires for the methods to have docstrings... the decorator implemented was not respecting the docstring of the wrapped function. That be easily fixed with functools.wraps (very useful as well).
    2ad65a1

@germa89
Copy link
Collaborator

germa89 commented Apr 14, 2025

@AlejandroFernandezLuces are we missing anything here?

Btw, title must be changed since the scope has expanded .

@AlejandroFernandezLuces
Copy link
Contributor Author

I was waiting for your feedback on the conversation about the decorator, from my side it is good to go as is. If you want to expand the current implementation of the decorator let me know.

Also, vulnerability check is failing, but it doesn't seem to be failing in anything I've modified..

@germa89
Copy link
Collaborator

germa89 commented Apr 14, 2025

I was waiting for your feedback on the conversation about the decorator, from my side it is good to go as is. If you want to expand the current implementation of the decorator let me know.

Also, vulnerability check is failing, but it doesn't seem to be failing in anything I've modified..

I just replied to the thread.

Do not bother about the vulnerability check. We will merge without it.

@germa89 germa89 enabled auto-merge (squash) April 14, 2025 12:14
@germa89 germa89 disabled auto-merge April 14, 2025 12:15
Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me.

One last request will be to have a warning/exception if an user is using the old vtk argument. I am concerned that this argument might be passed around in the kwargs without having effect. Please do also add a test for this check.
Probably this is a good case for a decorator.

@AlejandroFernandezLuces AlejandroFernandezLuces changed the title fix: Optional graphics dependency feat: Add optional graphical target and rework graphics backend selection Apr 14, 2025
@github-actions github-actions bot added the new feature Request or proposal for a new feature label Apr 14, 2025
Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

This PR is great! Thank you a lot @AlejandroFernandezLuces ! Great work!

@AlejandroFernandezLuces AlejandroFernandezLuces enabled auto-merge (squash) April 15, 2025 07:29
@AlejandroFernandezLuces AlejandroFernandezLuces merged commit f705902 into main Apr 15, 2025
45 of 47 checks passed
@AlejandroFernandezLuces AlejandroFernandezLuces deleted the fix/optional-visualizer branch April 15, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue, problem or error in PyMAPDL dependencies documentation Documentation related (improving, adding, etc) examples Publishing PyMAPDL examples maintenance General maintenance of the repo (libraries, cicd, etc) new feature Request or proposal for a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants