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 inheritance diagram to API reference #695

Merged
merged 8 commits into from Apr 24, 2020
Merged

Conversation

jeremyliweishih
Copy link
Contributor

@jeremyliweishih jeremyliweishih commented Apr 22, 2020

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Apr 22, 2020

ex:
Screen Shot 2020-04-22 at 4 30 20 PM

However when there is more super classes the nodes get small so needs one more fix.

@codecov
Copy link

codecov bot commented Apr 22, 2020

Codecov Report

Merging #695 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #695   +/-   ##
=======================================
  Coverage   99.19%   99.19%           
=======================================
  Files         140      140           
  Lines        4952     4952           
=======================================
  Hits         4912     4912           
  Misses         40       40           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09da1e6...173f217. Read the comment docs.

@jeremyliweishih jeremyliweishih self-assigned this Apr 23, 2020
@jeremyliweishih jeremyliweishih marked this pull request as ready for review Apr 23, 2020
@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Apr 23, 2020

@dsherry what I got so far. There should be more enhancements to make it look prettier but I didn't want to spend even more time on this!

@jeremyliweishih jeremyliweishih requested a review from dsherry Apr 23, 2020
@dsherry
Copy link
Collaborator

dsherry commented Apr 23, 2020

@jeremyliweishih I see RTD timed out on building this branch. But maybe that's a RTD problem rather than an issue with your code. I just rekicked it. Will review the code now and then approve once we can see the RTD output.

@dsherry
Copy link
Collaborator

dsherry commented Apr 23, 2020

@jeremyliweishih hmm, that build timed out too. Any idea what's up there? Very strange

@dsherry
Copy link
Collaborator

dsherry commented Apr 23, 2020

@jeremyliweishih one thing to note: the latest run of the build_docs job on this PR took 19min8sec. The latest run on master took 7min31sec. RTD will time out after about 20min of building, so it seems likely that RTD isn't the problem here, it's something in this PR causing an increased build time...

Update: wait a minute, I may be incorrect, looking again.

Yeah, never mind, the version on this branch took 8min25sec. Weird.

@dsherry
Copy link
Collaborator

dsherry commented Apr 23, 2020

@jeremyliweishih I saw the RTD passed for this, so I took a look. Pretty cool!! I'm amazed you got this working, haha. I feel like half the time I try to add something to sphinx I get burned.

I have some questions I'd like to hear your thoughts on:

  • Are there ever, or will there ever be, situations where the user needs to know the full inheritance structure, and not simply the class one step down?
  • Bearing that in mind, is this the most clear way to indicate inheritance? Would it be more clear to simply list the base class as I think you suggested originally?
  • Most of the inheritance plots require scrolling to view. Is there anything we could do to make this clear to users? I'm concerned people will just think the plots are cut off.

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Apr 23, 2020

@dsherry I went with this approach in the end mainly due to how many layers of inheritance we have now over the base class approach. The fact that the base class doesn’t link either makes it tedious to understand the inheritance structure we have.

Whether users actually need to understand full inheritance - I think it will be useful for components, pipelines, and objectives since it would be great for them to understand for custom components etc.. But I don’t don’t see a compelling reason for every other thing in the API reference. That’s why initially I thought it would be better to explain full inheritance structure in the walkthroughs as I don’t think it’s that useful to users to understand inheritance in general outside of the custom cases.

Yeah I agree that there’s more to be done to make the diagrams more clear - just wanted to get the review process going since I spent way too much time on it already haha

@dsherry
Copy link
Collaborator

dsherry commented Apr 24, 2020

@jeremyliweishih got it. I think you've convinced me the approach adds value. I do think we could improve the appearance, for instance I wonder if there's a way to have the graphs render vertically rather than horizontally. And I agree we should explain the inheritance and what's expected of users in our docs and not make them turn to the API ref to understand that. But for now, this is a value add and that's great. Nice going :) I'll review the PR.

img.inheritance {
max-width: none;
max-height: 30px;
}
Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

Why set specific pixel values for these things as opposed to percentages?

I mean, I guess I understand why the height is fixed at a pixel value. But I wonder if making it slightly more would be helpful. When I scroll to the right on my mac, the scroll bar covers half the image.

Screen Shot 2020-04-24 at 2 46 21 PM

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Apr 24, 2020

Choose a reason for hiding this comment

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

The real answer is: I couldn't get it to work with percentages lol. I wanted to use 100% of the div (max-height: 100%) but it never worked for me. Yeah I can bump the height value!

Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

Got it. Weird! That's ok.

@@ -2,6 +2,8 @@

.. currentmodule:: {{ module }}

.. inheritance-diagram:: {{ objname }}
Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

Which elements use class.rst?

Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

I guess I'm realizing I like that we have estimator.rst / transformer.rst because the names clearly indicate what those files correspond to, but not true with class.rst

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Apr 24, 2020

Choose a reason for hiding this comment

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

@dsherry its just every other class thats not pipeline/estimator/transformer. I made the other .rst files because of class properties.

Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

Oh, ok.

@@ -208,6 +210,8 @@
nbsphinx_execute = 'always'
nbsphinx_timeout = 600 # sphinx defaults each cell to 30 seconds so we need to override here

inheritance_graph_attrs = dict(rankdir="LR", size='"1000, 333"',
fontsize=30, labelfontsize=30, ratio='compress', dpi=960)
Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

Interesting. Is there doc on what parameters this module supports?

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Apr 24, 2020

Choose a reason for hiding this comment

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

Yeah! Its just the graphviz parameters. https://www.graphviz.org/doc/info/attrs.html

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Apr 24, 2020

Choose a reason for hiding this comment

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

We can play with them more later to make things prettier 😄

Copy link
Collaborator

@dsherry dsherry Apr 24, 2020

Choose a reason for hiding this comment

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

Nice

Copy link
Collaborator

@dsherry dsherry left a comment

I left some questions but it looks good to me!

Can you please verify that adding these graphs hasn't significantly increase our build_docs or RTD build runtime?

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Apr 24, 2020

@dsherry on this branch build time was 982s whereas master was 974s so it looks good!

@jeremyliweishih jeremyliweishih merged commit 360fc60 into master Apr 24, 2020
2 checks passed
@dsherry dsherry deleted the js_670_inherit branch Oct 29, 2020
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.

Show class inheritance in API reference
2 participants