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

[ENH, MRG] Make orphan of unused API entries #983

Merged
merged 76 commits into from
Aug 26, 2022

Conversation

alexrockhill
Copy link
Contributor

Fixes #974.

I just made a bulleted list of unused API entries and the percentage used so far and then a bulleted with sub-bullets for used API entries and where they are used. As discussed with @larsoner and @drammock, it would be nice to make them into graphs with graphviz but we would also have to handle lacking this dependency gracefully which I'm not as sure how to do. Pretty good start I think though!

Unused API Entries — Python documentation.pdf

@alexrockhill
Copy link
Contributor Author

Hmmm the Circle failure is quite odd and the test failure was because the backreferences file was a directory which I didn't know was allowed... would anyone have a minute to review this?

@larsoner
Copy link
Contributor

larsoner commented Aug 2, 2022

the Circle failure is quite odd

Sphinx does not give the nicest tracebacks. Have you tried make html in the doc dir of sphinx-gallery, then looking at the logs? You probably need to have encoding='utf8' somewhere...

the test failure was because the backreferences file was a directory

Have you looked at the structure it produces locally? My guess is that the backreferences files end up somewhere where some other files do (like auto_examples/whatever.py, hence having a directory plus some "stuff" inside it), so you can't assume everything in that dir is a backref .examples file or something...

@alexrockhill
Copy link
Contributor Author

Thanks @larsoner, that was really helpful, I think I figured it out!

@alexrockhill
Copy link
Contributor Author

alexrockhill commented Aug 2, 2022

I think this turned out really well!

You have to zoom in a bit since there are a lot of entries in some cases but I think super useful.

Unused API Entries — Python documentation.pdf

I'm not sure I handled graphviz importing and adding to the extensions in conf.py properly though so someone should double check that. Other than that though, I think ready to go!

@larsoner
Copy link
Contributor

larsoner commented Aug 2, 2022

From a quick look, code looks reasonable! But CIs are unhappy. Let me know when things are green or you need help interpreting

To demo this functionality, could you open a PR to MNE-Python demoing this functionality? You can tell CircleCI over there to use your branch in https://github.com/mne-tools/mne-python/blob/64dff6189215cbead61db81d6e6de811a05633e3/tools/circleci_dependencies.sh#L15

@alexrockhill
Copy link
Contributor Author

Hmmm, I'm getting this and similar warnings causing Circle to fail /home/circleci/project/doc/auto_examples/sg_api_usage.rst:17: WARNING: py:func reference target not found: numpy.ndarray. Clearly numpy.ndarray is a function and it's in the intersphinx mapping, anyone have any advice on how to fix this?

@drammock
Copy link
Contributor

drammock commented Aug 2, 2022

Clearly numpy.ndarray is a function and it's in the intersphinx mapping, anyone have any advice on how to fix this?

np.ndarray is a :class: not a :func:

@alexrockhill
Copy link
Contributor Author

Ah yeah that makes sense, it's warning for numpy.sin as well which I should double check but I'm pretty sure that one is a function. Is there a way to put the proper reference with just knowing the string? Maybe some kind of exec code? That seems a bit unideal though....

@larsoner
Copy link
Contributor

larsoner commented Aug 2, 2022

You could try :py:obj:, it might resolve to either of those

@alexrockhill
Copy link
Contributor Author

Ok now this seems like a bug with backreferences since it's trying to find numpy.RandomState when it was imported as np.random.RandomState so it should be numpy.random.RandomState in the backreferences examples file. I looked into it for a bit but it's quite complicated...

@alexrockhill
Copy link
Contributor Author

Ok there's two things I'm a bit stuck on: 1) The bug in the name resolving code (above) for numpy.random.RandomState and 2) how to remove modules that are not instantiated directly without removing classes (that also are camel case and share a root with their methods like modules share a root with their functions). Maybe if someone who wrote a bit of the name finding code could take a quick look, that might save a lot of time because the name finding is a bit dense.

@alexrockhill
Copy link
Contributor Author

Well I realized that modules should be lower case AND share a root with functions or classes in that module whereas classes should be camel case (and also share a root with their methods) so that fixes that. I'm not sure if the numpy.random.RandomState is symptomatic of a larger problem, although I would guess that it is but I just made an exception in this PR for now so that things pass.

sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
@alexrockhill
Copy link
Contributor Author

Ok, much more readable I think! I counted leaves instead of displaying for the ununsed API entries and broke the used API entries into sub-graphs, it's also much faster.

image

image

Trying on MNE now...

@alexrockhill
Copy link
Contributor Author

Now we're getting somewhere that's pretty useful!

Unused API entries in mne examples:

unused

Usage graph for Evoked:

evoked

@alexrockhill
Copy link
Contributor Author

Since the while section was made optional, I got rid of the details, I hope that's okay @larsoner. Let me know if you want to do it a different way.

@alexrockhill
Copy link
Contributor Author

Ok, this is good to merge by me

Copy link
Contributor

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

Thanks this is a great addition, quick review and mostly some doc nitpicks. I have not been following this closely so please excuse any comments that are not relevant/don't make sense.

doc/configuration.rst Outdated Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
will make one graph per module with all of the API entries connected to
the example that they are used in. This could be helpful for making a map
of where to look in a project if you want to learn about a particular
module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add that the graphviz package is required if you want the graphs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe adding an image of an example graph (e.g., of a sphinx gallery module) would be nice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the link to the sg_api_usage.html in the sphinx_gallery documentation, I think that might be a good way to show what it looks like!

sphinx_gallery/gen_gallery.py Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
@alexrockhill
Copy link
Contributor Author

Thanks this is a great addition, quick review and mostly some doc nitpicks. I have not been following this closely so please excuse any comments that are not relevant/don't make sense.

Thanks, I think there were some great clarifications!

@alexrockhill
Copy link
Contributor Author

Copy link
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM +1 for merge, feel free to merge when you're happy @lucyleeow !

Copy link
Contributor

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

Last thing, sorry!

doc/configuration.rst Outdated Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
Co-authored-by: Lucy Liu <jliu176@gmail.com>
@lucyleeow lucyleeow merged commit b31e78f into sphinx-gallery:master Aug 26, 2022
@lucyleeow
Copy link
Contributor

Thanks @alexrockhill and @larsoner !

@alexrockhill alexrockhill deleted the api branch August 26, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Add orphan html with unused API entries + graph?
4 participants