Skip to content
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

fix: Force render in example #2046

Merged
merged 60 commits into from
Jul 26, 2023
Merged

fix: Force render in example #2046

merged 60 commits into from
Jul 26, 2023

Conversation

AlejandroFernandezLuces
Copy link
Contributor

Explanation

Test if forcing the render of the example makes it work fine in the doc building in the CI. Currently, this fixes the issues with the visualization described in #1579 in the python visualizer.

GIF generation with pyvista is not working properly in my local (maybe there is another issue with that), so I'll give it a try with the CI.

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #2046 (dbc8bd3) into main (997e288) will increase coverage by 6.32%.
Report is 36 commits behind head on main.
The diff coverage is 94.94%.

@@            Coverage Diff             @@
##             main    #2046      +/-   ##
==========================================
+ Coverage   80.82%   87.14%   +6.32%     
==========================================
  Files          44       45       +1     
  Lines        7489     8208     +719     
==========================================
+ Hits         6053     7153    +1100     
+ Misses       1436     1055     -381     

@germa89
Copy link
Collaborator

germa89 commented May 8, 2023

@AlejandroFernandezLuces I downloaded the zip and it doesn't show the gif.

image

@AlejandroFernandezLuces
Copy link
Contributor Author

@germa89 aside from this GIF, aren't you having issues with building the documentation? In this PR I reverted all of the changes, until this branch was the same as main, and the docs weren't building properly 50f8357 , until @RobPasMue added its fix.

In my local I'm not being able to build the docs, and I think @clatapie isn't being able to do so either.

Regarding the GIF, I'm honestly at a loss on what's happening. It is working fine when running manually, and other gifs work fine in the docs as well, it's just this example. I'll keep making tests on this PR to see if I can find the source of the error.

@RobPasMue
Copy link
Member

@germa89 @AlejandroFernandezLuces

It renders to me...

video.webm

@AlejandroFernandezLuces
Copy link
Contributor Author

AlejandroFernandezLuces commented May 9, 2023

From my perspective we have a couple of issues that are not straightforward to fix:

  • There are unknown issues in PyMAPDL doc building. The change @RobPasMue did to suppress warnings seems to have fixed it in this PR, but how is it working in main? Also, in local it doesn't seem to work too well either.
  • The meshing is not being rendered correctly for the docs, while when running the example and you visualize it with the python plotter it work nice. This is the original issue that I try to fix in this PR, but it might be caused by one of the other issues.
  • pyvista image scrapper for sphinx is generating the GIFs with PNG extensions. This may cause some engines to not show the GIFs properly. For example: PyMAPDL GIF and PyVista GIF

It's hard to know where the meshing issue comes from. I'll give it a few more tries with PyVista options, but if that isn't working we'll have to fix the other issues first. I opened an issue in PyVista regarding the extensions: pyvista/pyvista#4397.

@AlejandroFernandezLuces
Copy link
Contributor Author

The PNG issue is fixed in pyvista/pyvista#4403 , we can wait unit it's patch released to continue working here. Let's hope that helps.

@RobPasMue
Copy link
Member

It was patched released this weekend - update the reqs and merge :)

@AlejandroFernandezLuces
Copy link
Contributor Author

What's the status here @AlejandroFernandezLuces ?

I ran out of ideas on this one, my last comment is the best I could get out of the documentation. I'll triple check again once I get home, but I think that's the best I could do.

If it's acceptable, we can manually add this particular gif to the documentation, since it is working fine with regular plotting and it is the only gif that is failing.

@RobPasMue
Copy link
Member

Let's try to close this up with the best solution possible. Double check when you can @AlejandroFernandezLuces

@germa89
Copy link
Collaborator

germa89 commented Jul 24, 2023

Any update on this @AlejandroFernandezLuces ??

@AlejandroFernandezLuces
Copy link
Contributor Author

Yes, I added the GIF manually to the documentation. It is not the best solution, but at least it will show properly in the documentation.

Another approach I tried, since it would be better for maintenance, is to run the example before building the docs and getting the GIF from there, but it wasn't working for some reason. Please @germa89 review and let me know if this approach is good enough.

@AlejandroFernandezLuces AlejandroFernandezLuces marked this pull request as ready for review July 24, 2023 15:49
@germa89
Copy link
Collaborator

germa89 commented Jul 25, 2023

I was kind of conflicted with this. Hiding the issue is not good. But it is probably not worthy (at the moment) to keep investing time in such small impact issue. I think probably the issue is on Pyvista side. I check the animation and it seems there are some peak values, that is the animation camera is so far away. However I checked manually the values used to update the gif and they seems OK. So probably the error is when updating the values under a sphinx environment. Again, very small impact issue.

So I'm approving this PR. Thank a lot for the time you spent on this @AlejandroFernandezLuces, I know it has been a lot.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
examples/00-mapdl-examples/composite_dcb.py Outdated Show resolved Hide resolved
examples/00-mapdl-examples/composite_dcb.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
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.

LGTM. Thank you @AlejandroFernandezLuces !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants