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

Add gif extension to Sphinx-Gallery scraper #4403

Merged
merged 6 commits into from
May 11, 2023
Merged

Conversation

AlejandroFernandezLuces
Copy link
Contributor

Overview

Sphinx image scrapper is returning GIF files with PNG extension. This PR fixes it.

Closes #4397

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label May 10, 2023
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #4403 (4587e65) into main (461bb5e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4403   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files          97       97           
  Lines       20874    20875    +1     
=======================================
+ Hits        20002    20004    +2     
+ Misses        872      871    -1     

@adeak adeak changed the title Add gif extension to Sphinx image scrapper Add gif extension to Sphinx image scraper May 10, 2023
@AlejandroFernandezLuces AlejandroFernandezLuces marked this pull request as ready for review May 10, 2023 13:26
@AlejandroFernandezLuces
Copy link
Contributor Author

This should be good to go, @akaszynski can you take a look?

@akaszynski akaszynski added this to the v0.39.1 milestone May 10, 2023
Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

This was intentional at one point -- I need to come back to this when I have more time and look into this.

Another great person to review would be @larsoner

@banesullivan banesullivan changed the title Add gif extension to Sphinx image scraper Add gif extension to Sphinx-Gallery scraper May 10, 2023
@akaszynski
Copy link
Member

This was intentional at one point -- I need to come back to this when I have more time and look into this.

We've had problems in the past generating latex documentation when using gifs. I think we purposely changed the extension to 'png' to avoid this issue.

@banesullivan
Copy link
Member

More than just LaTeX if I remember correctly -- I think it had side effects on the preview cards on the gallery page too. I remember there being a few issues. The sphinx-gallery team would know how to best handle this

@AlejandroFernandezLuces
Copy link
Contributor Author

I didn't consider those cases, thanks for pointing that out! I just was thinking that maybe some browsers can't identify that .png files are actually GIFs and they wouldn't show properly.

@larsoner
Copy link
Contributor

SG should handle GIFs fine on gallery pages nowadays

https://mne.tools/dev/auto_tutorials/index.html#source-localization-and-inverses

@larsoner
Copy link
Contributor

... no idea on the LaTeX front, though. To me if it's not handled properly it should probably be considered a Sphinx (not sphinx-gallery) issue -- it seems like Sphinx should detect that it's not LaTeX-friendly and just take the first frame and embed as PNG or something. But if this is too onerous for them we could triage based on builder type in SG (ugly but would work).

@akaszynski
Copy link
Member

More than just LaTeX if I remember correctly -- I think it had side effects on the preview cards on the gallery page too. I remember there being a few issues. The sphinx-gallery team would know how to best handle this

According to Sphinx Gallery - Advanced usage, these guys support it.

Just checked a local build and downloaded the artifacts... this PR doesn't rename the gif correctly and it's still showing up as a 'png':
sc001

@AlejandroFernandezLuces
Copy link
Contributor Author

AlejandroFernandezLuces commented May 10, 2023

In commit 09de6c0 it's working well, so it must still be getting the examples from the cache. I'm not sure how to reset it, or if that's even possible for just this PR.
image

@akaszynski
Copy link
Member

In commit 09de6c0 is working well, so it must still be getting the examples from the cache. I'm not sure how to reset it, or if that's even possible for just this PR.

Weird, because I cleaned everything locally. I'm going to push a trivial commit to ensure the GIF example is rebuilt.

@akaszynski
Copy link
Member

akaszynski commented May 10, 2023

Locally verified this works. Environment issue on my end:
sc002

GitHub is having a rough day, I'll ensure we get this to run properly.

@akaszynski
Copy link
Member

Gallery looks fantastic as well:
Examples — PyVista 0 40 dev0 documentation

Really great to see animated thumbnails.

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @AlejandroFernandezLuces.

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

This is really fantastic! Thanks a lot :)

@larsoner
Copy link
Contributor

Let me know if there is any remaining ambiguity from the SG end but otherwise +1 for merge. Agreed the GIFs look nice 👀 🍬

@akaszynski akaszynski merged commit 438dd0d into main May 11, 2023
@akaszynski akaszynski deleted the fix/gif-docs-generation branch May 11, 2023 20:16
akaszynski added a commit that referenced this pull request May 16, 2023
* Add gif extension

* Reset cache

* Add test for GIFs

* Revert cache regeneration

* Fix test

* use cells for the example

---------

Co-authored-by: Alex Kaszynski <[email protected]>
@akaszynski akaszynski mentioned this pull request May 16, 2023
3 tasks
akaszynski added a commit that referenced this pull request May 16, 2023
* Add PyHyperbolic3D as an external example (#4420)

* Add PyHyperbolic3D as an external example

* Reduce Gif file size

* Use 128 color

* optimize and scale

---------

Co-authored-by: Alex Kaszynski <[email protected]>

* Fix merge dataset (#4414)

* convert back to polydata using extact_geometry

* improved readability

* branch for performance

* add in verts

* revert #4407 into this branch

* Avoided omission of two letters.

---------

Co-authored-by: Tetsuo Koyama <[email protected]>

* Remove SKIP in docstrings (#4419)

* remove SKIPs

* remove the rest of skips

* minor fixes

* Fix active scalars and pass point data in `to_tetrahedra` for `RectilinearGrid` (#4406)

* add failing test

* fix active scalars for cell data

* pass point data, deprecate use of pass_cell_data

* document parameter

---------

Co-authored-by: Alex Kaszynski <[email protected]>

* Add gif extension to Sphinx-Gallery scraper (#4403)

* Add gif extension

* Reset cache

* Add test for GIFs

* Revert cache regeneration

* Fix test

* use cells for the example

---------

Co-authored-by: Alex Kaszynski <[email protected]>

* bump version to v0.39.1

---------

Co-authored-by: Tetsuo Koyama <[email protected]>
Co-authored-by: MatthewFlamm <[email protected]>
Co-authored-by: Alex Fernandez <[email protected]>
@banesullivan banesullivan mentioned this pull request Jun 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GIFs are generated with PNG extension in documentation
5 participants