pymapdl icon indicating copy to clipboard operation
pymapdl copied to clipboard

feat: Adapt PyMAPDL to common plotter

Open AlejandroFernandezLuces opened this issue 1 year ago โ€ข 3 comments

Overview

This PR adapts PyMAPDL to the usage of the common PyAnsys plotter. It seems that part of PyMAPDL plotter is in another repo, so this work is partially done until I figure out how to modify this other part.

To summarize, on top of improving maintainability by keeping some common plotting code in the same location, this adds the following features:

  • Change view buttons
  • Displacement buttons
  • Ruler widget to measure distances in the plotter
  • Measure box widget
  • Option to filter the meshes by a regex
  • Option to add custom buttons to the plotter

Screenshot of how the plotter looks now:

image

Missing:

  • [ ] Document and add comments where needed
  • [ ] Adapt the other part of PyMAPDL
  • [ ] Adapt picker to use common plotter picker

AlejandroFernandezLuces avatar Feb 20 '24 15:02 AlejandroFernandezLuces

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

@ansys-reviewer-bot review

ansys-reviewer-bot[bot] avatar Feb 20 '24 15:02 ansys-reviewer-bot[bot]

@AlejandroFernandezLuces how are we doing with this?

germa89 avatar May 17 '24 10:05 germa89

@AlejandroFernandezLuces how are we doing with this?

The package was open source released last week, there were some major changes so I'll have to readapt as soon as I can. Right now I'm OoO, the following weeks I'll start readapting repos to the new visualization interface ๐Ÿ˜

AlejandroFernandezLuces avatar May 17 '24 11:05 AlejandroFernandezLuces

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jun 03 '24 09:06 github-actions[bot]

Codecov Report

Attention: Patch coverage is 91.11748% with 31 lines in your changes missing coverage. Please review.

Project coverage is 83.09%. Comparing base (03f722e) to head (7c3bbd4). Report is 1 commits behind head on main.

:exclamation: There is a different number of reports uploaded between BASE (03f722e) and HEAD (7c3bbd4). Click for more details.

HEAD has 78 uploads less than BASE
Flag BASE (03f722e) HEAD (7c3bbd4)
dmp 16 6
minimal 6 3
ubuntu 20 9
local 13 6
non-student 15 0
smp 8 3
remote 11 3
v23.2-ubuntu 3 0
v24.1-ubuntu 3 0
centos 4 0
v24.2.0 1 0
v24.2-ubuntu 2 0
latest-ubuntu 3 0
v24.1.0 1 0
v25.1.0 1 0
v23.1.0 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2799      +/-   ##
==========================================
- Coverage   88.22%   83.09%   -5.13%     
==========================================
  Files          53       55       +2     
  Lines        9670     9753      +83     
==========================================
- Hits         8531     8104     -427     
- Misses       1139     1649     +510     

codecov[bot] avatar Jun 03 '24 09:06 codecov[bot]

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jun 03 '24 10:06 github-actions[bot]

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jun 03 '24 10:06 github-actions[bot]

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jun 04 '24 10:06 github-actions[bot]

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jun 12 '24 07:06 github-actions[bot]

Everything seems to be working fine except for the minimal test jobs. I'm not sure what's happening there, any insights @germa89?

https://github.com/ansys/pymapdl/actions/runs/9481844411/job/26125469222

GitHub
Pythonic interface to MAPDL. Contribute to ansys/pymapdl development by creating an account on GitHub.

AlejandroFernandezLuces avatar Jun 12 '24 11:06 AlejandroFernandezLuces

All CI tests are passing now, should I worry about the patch coverage? I'm not sure if it makes much sense to test the few lines that are not covered.

AlejandroFernandezLuces avatar Jun 14 '24 10:06 AlejandroFernandezLuces

@germa89 Did you have the chance to take a look at this PR?

AlejandroFernandezLuces avatar Jun 18 '24 08:06 AlejandroFernandezLuces

All CI tests are passing now, should I worry about the patch coverage? I'm not sure if it makes much sense to test the few lines that are not covered.

Regarding this, I rather you do some unit tests to increase the coverage up to 90%. @AlejandroFernandezLuces

germa89 avatar Jun 18 '24 11:06 germa89

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jul 02 '24 10:07 github-actions[bot]

How's this going @AlejandroFernandezLuces @germa89?

RobPasMue avatar Jul 03 '24 07:07 RobPasMue

How's this going @AlejandroFernandezLuces @germa89?

I did an important refactor of the PR to have a better integration of the visualization tool. This solved some of @germa89 comments.

Right now I'm missing two things, to check why one specific picking test is failing, and why some of the images from the cache are different.

AlejandroFernandezLuces avatar Jul 03 '24 08:07 AlejandroFernandezLuces

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jul 08 '24 07:07 github-actions[bot]

@germa89 Can you give a review again?

AlejandroFernandezLuces avatar Jul 08 '24 12:07 AlejandroFernandezLuces

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jul 09 '24 07:07 github-actions[bot]

@AlejandroFernandezLuces check also that https://github.com/ansys/pymapdl/pull/2799/commits/3348249f01b502d957ab57cb610dac5b683c2920 is correct.

germa89 avatar Jul 09 '24 10:07 germa89

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jul 09 '24 10:07 github-actions[bot]

A side discussion, code is somewhat coupled with PyVista due to how the picker works in PyMAPDL. I would suggest refactoring the picking capabilites of PyMAPDL in a follow up PR to be more understandable and to adapt to the visualization tool better. IMO, we could do the following:

  • Change the "push X key and then picking will do Y" to a set of graphic buttons on the side that select the function you want.
  • Split the current picking code into the callbacks we need, and associate the callbacks to the picker on graphical button click.
  • Right now we are assigning a plotter to the callback, which seems to me that is doing things in reverse. I would suggest to write it the other way, associate the callbacks to the plotter.

AlejandroFernandezLuces avatar Jul 09 '24 12:07 AlejandroFernandezLuces

Regarding the font, PyVista only allows three: courier, which is the default one, arial and times. I think the one you are looking for is arial, if not, please let me know how to set a different font. I'm too having issues with the labels in particular, I've set the family font of the labels to arial, let's see if that works.

Regarding the transparency, I think it appears different because the screenshots are a bit more blurry with the new changes, probably because of the buttons. The opacity is 0.5 in my changes and in main branch.

AlejandroFernandezLuces avatar Jul 09 '24 15:07 AlejandroFernandezLuces

A side discussion, code is somewhat coupled with PyVista due to how the picker works in PyMAPDL. I would suggest refactoring the picking capabilites of PyMAPDL in a follow up PR to be more understandable and to adapt to the visualization tool better. IMO, we could do the following:

  • Change the "push X key and then picking will do Y" to a set of graphic buttons on the side that select the function you want.
  • Split the current picking code into the callbacks we need, and associate the callbacks to the picker on graphical button click.
  • Right now we are assigning a plotter to the callback, which seems to me that is doing things in reverse. I would suggest to write it the other way, associate the callbacks to the plotter.

I agree with this. Totally. Let's do it in a follow up PR.

germa89 avatar Jul 09 '24 15:07 germa89

Regarding the font, PyVista only allows three: courier, which is the default one, arial and times. I think the one you are looking for is arial, if not, please let me know how to set a different font. I'm too having issues with the labels in particular, I've set the family font of the labels to arial, let's see if that works.

Regarding the transparency, I think it appears different because the screenshots are a bit more blurry with the new changes, probably because of the buttons. The opacity is 0.5 in my changes and in main branch.

It is not big deal though. Do whatever you can, and move on.

germa89 avatar Jul 09 '24 15:07 germa89

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jul 10 '24 09:07 github-actions[bot]

Hello! :wave:

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected โ€” Waiting for status to be reported. Do not worry. You commit workflow is still running here :smile:

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. :nerd_face:

github-actions[bot] avatar Jul 10 '24 11:07 github-actions[bot]

@germa89 take a look whenever you have a chance ๐Ÿ™‚

AlejandroFernandezLuces avatar Jul 10 '24 13:07 AlejandroFernandezLuces

Not sure how to force the bot to rename the changelog file, I can't do it manually, do you know how @clatapie?

AlejandroFernandezLuces avatar Jul 15 '24 08:07 AlejandroFernandezLuces

Since this is a big change, I'll wait a bit more before merging just in case anyone has any more feedback. Thanks @germa89 @clatapie @PipKat for your feedback ๐Ÿš€

AlejandroFernandezLuces avatar Jul 15 '24 10:07 AlejandroFernandezLuces