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

DOC: Add docstrings to Filter mapping views #7075

Merged
merged 8 commits into from Nov 6, 2023
Merged

DOC: Add docstrings to Filter mapping views #7075

merged 8 commits into from Nov 6, 2023

Conversation

akshayamadhuri
Copy link
Contributor

issue no: #4433

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I think the only issue here is white-space changes identified in the linting check.
The indentation should match on each line, and there might be some space at the end of a line or two.

Running black locally on this file should fix it too if your prefer.

Do these doc_strings show up anywhere in the online documentation? I don't think we've hooked into that. The way to do that is to add lines to the doc/reference collection of rst files. Maybe with a section of the classes that tries to cover all the views. That is perhaps beyond the scope of this PR. We can merge this to get the doc_strings in there and then do the other stuff in another PR. Or... if you want to take a stab at an rst file about views in this PR that's fine too.

First step is to get the tests to pass.

@rossbar
Copy link
Contributor

rossbar commented Nov 3, 2023

Do these doc_strings show up anywhere in the online documentation?

Good news - they do indeed! The Filter* classes are already included in the coreviews autosummary. For example compare the autosummary from this PR to the devdocs.

Similarly the class docs are autorendered as well: this PR vs. devdocs

There's certainly some tweaking that could be done; starting with formatting with cleanup, but the overall approach is good!

@dschult
Copy link
Member

dschult commented Nov 3, 2023

Thanks @rossbar! That's great!!
They look much better already. :)
But you are right that there is probably still work to do for completeness and formatting.

@akshayamadhuri Let us know if you want to keep going with this PR. We can use a separate PR if you prefer.

@akshayamadhuri
Copy link
Contributor Author

akshayamadhuri commented Nov 3, 2023

@dschult @rossbar thanks for the review, sure! i am also working here class Docs

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Having read more about how the Filter* classes are used in NetworkX, I couldn't help but wonder whether it might be better to remove them from the documentation instead. I get the sense that the filter views are really only intended to be used internally to implement subgraph views instead of being used in user-code directly.

@dschult @MridulS any thoughts on this? Are there applications where users would be expected/benefit from creating Filter* objects directly?

@dschult
Copy link
Member

dschult commented Nov 5, 2023

The way I remember it, the filter classes were created to enable us to implement subgraph views, etc. And the Filter functions (in filters.py) were meant to cover the commonly desirable filters and also as examples to how users could build filter functions to do unusual or exotic filtering of edges and nodes. So the Filter* classes are not expected to be used directly by users, the filters themselves are expected to be used and morphed by users.

That said, the other "core views" like AtlasView and UnionAtlas are in that same category -- probably shouldn't be used directly by the users. So either all of those should be included in the docs, or none of them. I guess I lean toward providing at least the simple kind of docs we have for those classes (and the simple docs created by this PR). I don't see a downside to it (but maybe I am missing it). I'm sure there are people who dive into the code and want to know more about the underlying data structures. Maybe it is worth including full documentation for those folks, but I'm not sure. They probably want to read the code anyway. The downside is too much effort spent making those doc_strings complete and well written.

I'm open to suggestions for how to proceed. It makes sense to me to get this PR clean and formatted, but not to worry about including full doc_strings or the "core views" (including Filter*).

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Thanks for the insight @dschult ! In that case I took the liberty of pushing up a few minor changes to get this over the line; namely:

  • Use literal formatting for the NODE/EDGE_OK references, and
  • Remove the summary lines from the See Also listings (the links go directly to these summaries)

Thanks for the improvements @akshayamadhuri !

Copy link
Member

@MridulS MridulS left a comment

Choose a reason for hiding this comment

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

Thanks @akshayamadhuri !

@MridulS
Copy link
Member

MridulS commented Nov 6, 2023

I think this is "good enough" for docs for internal Views implementation. Anyone who is using this will have to jump into the codebase anyway. This PR gives a good enough starting point (IMO) if someone ends up on the documentation page of Filter*View, definitely an improvement over a blank doc page.

@MridulS MridulS changed the title Update coreviews.py DOC: Add docstrings to Filter mapping views Nov 6, 2023
@MridulS MridulS merged commit 9aa44be into networkx:main Nov 6, 2023
35 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Nov 6, 2023
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* Update coreviews.py

* Update coreviews.py

* Update coreviews.py

* Update coreviews.py

* Update coreviews.py

* Update coreviews.py

* Update coreviews.py

* Rm summary lines from see-alsos.

---------

Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants