pymapdl
pymapdl copied to clipboard
feat: Adapt PyMAPDL to common plotter
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:
Missing:
- [ ] Document and add comments where needed
- [ ] Adapt the other part of PyMAPDL
- [ ] Adapt picker to use common plotter picker
Thanks for opening a Pull Request. If you want to perform a review write a comment saying:
@ansys-reviewer-bot review
@AlejandroFernandezLuces how are we doing with this?
@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 ๐
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:
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
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:
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:
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:
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:
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.
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.
@germa89 Did you have the chance to take a look at this PR?
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
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:
How's this going @AlejandroFernandezLuces @germa89?
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.
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:
@germa89 Can you give a review again?
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:
@AlejandroFernandezLuces check also that https://github.com/ansys/pymapdl/pull/2799/commits/3348249f01b502d957ab57cb610dac5b683c2920 is correct.
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:
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.
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.
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.
Regarding the font, PyVista only allows three:
courier
, which is the default one,arial
andtimes
. I think the one you are looking for isarial
, 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.
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:
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:
@germa89 take a look whenever you have a chance ๐
Not sure how to force the bot to rename the changelog file, I can't do it manually, do you know how @clatapie?
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 ๐