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

refactor: refactor visualisations #56

Merged
merged 103 commits into from
Apr 11, 2023
Merged

refactor: refactor visualisations #56

merged 103 commits into from
Apr 11, 2023

Conversation

yuxqiu
Copy link
Collaborator

@yuxqiu yuxqiu commented Apr 9, 2023

Address #53, #54

@TrueLearnAI TrueLearnAI deleted a comment from codecov-commenter Apr 9, 2023
docs/conf.py Show resolved Hide resolved
docs/dev/testing.rst Outdated Show resolved Hide resolved
@yuxqiu
Copy link
Collaborator Author

yuxqiu commented Apr 11, 2023

The problem now is that I don't know why the Sphinx Gallery isn't working properly (It seems that it does not correctly use env.rate). So, we can't build the documentation correctly now.

pyproject.toml Show resolved Hide resolved
@KD-7
Copy link
Contributor

KD-7 commented Apr 11, 2023

image
You can see where you are missing lines in coverage here

image
After fixing these 2 we will have 100% coverage

@yuxqiu
Copy link
Collaborator Author

yuxqiu commented Apr 11, 2023

image You can see where you are missing lines in coverage here

image After fixing these 2 we will have 100% coverage

I don't think it's possible to cover the second one because we're running coverage tests in Python 3.7. The code snippet in the second image will only be runned if you have a Python version greater than 3.7.

I suggest we add a # pragma: no cover there.

@KD-7
Copy link
Contributor

KD-7 commented Apr 11, 2023

The problem now is that I don't know why the Sphinx Gallery isn't working properly (It seems that it does not correctly use env.rate). So, we can't build the documentation correctly now.

Yes, the problem persists on local machine

@yuxqiu
Copy link
Collaborator Author

yuxqiu commented Apr 11, 2023

The problem now is that I don't know why the Sphinx Gallery isn't working properly (It seems that it does not correctly use env.rate). So, we can't build the documentation correctly now.

Yes, the problem persists on local machine

@KD-7 Found a dirty but effective solution. My guess is that some other plugin is interfering with the way sphinx gallery works. If I adjust the position of the extension so that sphinx gallery is the first one (as shown below), it works without any exceptions. But the images are not generated correctly (this is because of our example code).

# in conf.py

# this list determines the order in which these extensions are added
extensions = [
    "sphinx_gallery.gen_gallery",
    "sphinx.ext.autodoc",
    "sphinx.ext.linkcode",
    "sphinx.ext.autosummary",
    "sphinx.ext.napoleon",
    "sphinx.ext.intersphinx",
    "sphinx.ext.doctest",
    "sphinx_copybutton",
]

Edit.

It seems that if we put sphinx.ext.autosummary under sphinx_gallery.gen_gallery, it works fine. However, I think sklearn also puts sphinx.ext.autosummary above sphinx_gallery.gen_gallery like what we did before and they don't encounter any problems.

@yuxqiu
Copy link
Collaborator Author

yuxqiu commented Apr 11, 2023

The reason why image is not shown is because of their SphinxGalleryOrcaRenderer.

If we use plotly.io.show(plotter.figure) to show our the visualisation, it will correctly generate images. However, if we use plotter.show(), it will not generate images in the correct folder because inside their SphinxGalleryOrcaRenderer (which is used to render plotly output if we use sphinx_gallery), they get the "filename" based on the stack level:

class SphinxGalleryOrcaRenderer(ExternalRenderer):
    def render(self, fig_dict):
        stack = inspect.stack()
        # Name of script from which plot function was called is retrieved
        try:
            filename = stack[3].filename  # let's hope this is robust...
        except:  # python 2
            filename = stack[3][1]
        filename_root, _ = os.path.splitext(filename)
        filename_html = filename_root + ".html"
        filename_png = filename_root + ".png"
        ...

In our case, since we are not calling plotly.io.show directly, the filename we get is incorrect.

Note: When we use our plotter.show, the stack looks like this:

SphinxGalleryOrcaRenderer.render
---
renderers._perform_external_rendering
---
plotly.io.show
---
figure.show
---
plotter.show
---
callee

If we call plotly.io.show directly, it looks like this

SphinxGalleryOrcaRenderer.render
---
renderers._perform_external_rendering
---
plotly.io.show
---
callee

We can see that for the first one, stack[3] is figure.show, which is a method inside plotly itself. So, all the images are generated in its folder.

@yuxqiu
Copy link
Collaborator Author

yuxqiu commented Apr 11, 2023

@KD-7 Ready for review again.

Copy link
Contributor

@KD-7 KD-7 left a comment

Choose a reason for hiding this comment

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

Excellent work, we just need to add two things:

  • Examples for all the plotters
  • Design considerations for visualisations

@yuxqiu
Copy link
Collaborator Author

yuxqiu commented Apr 11, 2023

Excellent work, we just need to add two things:

  • Examples for all the plotters
  • Design considerations for visualisations

Let's do this in two new PRs.

@yuxqiu yuxqiu merged commit d5274a2 into main Apr 11, 2023
@yuxqiu yuxqiu deleted the refactor-vis branch April 11, 2023 18:01
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.

None yet

3 participants