-
Notifications
You must be signed in to change notification settings - Fork 142
fix(plotting): Improve interface of the plotting class. #3702
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
Conversation
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
… into fix/plotter-issues
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3702 +/- ##
==========================================
- Coverage 88.38% 88.37% -0.01%
==========================================
Files 187 187
Lines 14742 14751 +9
==========================================
+ Hits 13029 13036 +7
- Misses 1713 1715 +2 🚀 New features to boost your workflow:
|
|
@clatapie This PR fixes your original issue. I'm unsure if more issues will arise, since plots are not throwing errors and I do not know how plots should look like. I would need to compare results from the original examples to what we have right now for further debugging. From my point of view, we can either merge this PR and we can add more fixes as issues arise, or you can install this branch in your local and ping me when something does not look as expected. What do you think? |
|
Thanks for fixing the error @AlejandroFernandezLuces! |
|
@AlejandroFernandezLuces any update on this? Is this planned? |
|
This is pending @clatapie PR in ansys/pymapdl-examples#281. This PR intends to fix some issues that arise in that PR. @clatapie let me know if you need any help 🙂 |
|
I confirm that the issue has correctly been addressed with this fix. Thank you @AlejandroFernandezLuces! |
|
This PR needs a bit more coverage before being merged. |
germa89
left a comment
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.
Please increase the coverage before merging. Plotting is an important feature and needs to be as much tested as possible.
|
@germa89 increased coverage to 100%. I'm seeing an issue with the CI in this PR, it seems that there is a required job that does not exist: Is this an specific problem for this PR or is it general? |
germa89
left a comment
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.
LGTM!
@AlejandroFernandezLuces it was my fault. :) |


Description
This PR improves the usability of some methods in the plotting class. This class was meant to be used inside eplot, vplot.. functions, but since some use cases involve using it standalone, I improved it's usability and documentation.
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)