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

First shot at the interface #9

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

etiennedeg
Copy link
Member

@etiennedeg etiennedeg commented Aug 31, 2023

Try to implement JuliaGraphs/Graphs.jl#146

@etiennedeg etiennedeg changed the title first commit First shot at the interface Aug 31, 2023
@etiennedeg etiennedeg marked this pull request as draft August 31, 2023 06:58
@gdalle
Copy link
Member

gdalle commented Aug 31, 2023

👀

@gdalle gdalle self-requested a review August 31, 2023 07:09
@gdalle gdalle self-assigned this Aug 31, 2023
@gdalle
Copy link
Member

gdalle commented Aug 31, 2023

@etiennedeg ping me when you want me to make a first pass. I can also fix CI if you want, looks like mostly docstring stuff

@gdalle
Copy link
Member

gdalle commented Sep 1, 2023

Why add Graphs in the test deps?

@etiennedeg
Copy link
Member Author

For generating the docstrings, I feel it would be weird to have using GraphsBase rather than Graphs in the docstring, plus some of the functions used in the docstring will not be part of GraphsBase. Of course, we need to have the 2.0 branch of Graphs updated and importing GraphsBase (and we will add the 2.0 branch as a dependency for the time of development)

@gdalle
Copy link
Member

gdalle commented Sep 1, 2023

I don't agree with this. GraphsBase will be a standalone package, and should only rely on its own functions for documentation and testing. The downstream testing with graph algorithms should happen in Graphs itself, and not be upstreamed to GraphsBase. If Graphs breaks, we'll fix GraphsBase anyway

@etiennedeg
Copy link
Member Author

I'm not speaking of testing nor the global documentation, only for the docstring, the main users will only import Graphs.jl and will use ? to get the documentation of the main methods, which are nv, neighbors, etc... It would be weird that the provided examples show using GraphsBase rather than Graphs. Also, some docstrings generate use a generator to get a graph (for example they call path_graph), but these generators won't be in GraphsBase.

The potential users of GraphsBase will already be a bit familiar with Graphs. If they want to use GraphsBase, they will have a documentation page for GraphsBase that explain what is contained in this library. They will not be worried that the docstrings call using Graphs.

@gdalle
Copy link
Member

gdalle commented Sep 1, 2023

Okay that seems fair to me, although I wish we could do it in a cleaner way

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.

2 participants