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

Consistent way to specify algorithm/method #559

Open
szhorvat opened this issue Aug 10, 2022 · 5 comments
Open

Consistent way to specify algorithm/method #559

szhorvat opened this issue Aug 10, 2022 · 5 comments
Labels
todo Triaged for implementation in some unspecified future version

Comments

@szhorvat
Copy link
Member

Some functions have several selectable implementation, or can perform an operation with several methods / algorithms. There should be a single predictable keyword argument name to select the method to use. Now different functions use different names, such as implementation (pagerank), method (Degree_Sequence) or algo (layout selection).

@ntamas I assume you want to postpone this until the stimulus-based rewrite? Can you add it to the appropriate milestone?

Related: igraph/rigraph#526

@ntamas ntamas added this to the 0.10.0 milestone Aug 10, 2022
@ntamas
Copy link
Member

ntamas commented Aug 10, 2022

Adding this to 0.10.0 for the time being as I don't see yet how the stimulus-based rewrite will tie into the python-igraph repo. It might end up being such a fundamental change in API and everything that we'll need to think carefully about how to manage the transition.

As for t he argument naming -- in the stimulus-based rewrite (re-generation), the argument names will be taken directly from functions.yaml on the C side, so any inconsistencies should be fixed there.

@szhorvat
Copy link
Member Author

As for t he argument naming -- in the stimulus-based rewrite (re-generation), the argument names will be taken directly from functions.yaml on the C side, so any inconsistencies should be fixed there.

Isn't that too much of a burden on the C side? It's can't be pythonic enough and R-like enough and also suit Mathematica. There should be a higher-level layer in each of the host languages that makes the interface fit the host language well enough.

For example, in Mathematica, some of the things that are exposed as a "method" map to an enum, but some others map to choices of different functions (unweighted, Dijkstra, Bellman-Ford shortest paths). Mathematica supports method "sub-options" so sometimes separate C functions will be the only way to go.

It'll take some time until we get there, but right now I am skeptical that it will be possible to create a C interface that can be automatically converted to a convenient high-level interface. I am worried that the high-level interface we get this way might:

  • feel too low-level
  • not fit some languages
  • impose inconvenient constraints on the C side

For now, this is just a thought. We'll need to see how it goes in practice.

@ntamas
Copy link
Member

ntamas commented Aug 10, 2022

I'm not thinking about completely doing away with a middle layer that provides "pythonic" access to igraph's core functions, I'm just trying to minimize the amount of code we need to write to implement that middle layer. Fixing the argument naming inconsistencies on the C side is one way of minimizing the amount of hand-written code (i.e. if the only reason why a function from igraph's C core cannot be exposed directly on the Python side is the argument naming, there is no need to write a Python function just to translate the argument names).

We should continue this discussion on Discord Slack, though, to keep the issue focused.

@ntamas ntamas modified the milestones: 0.10.0, 0.10.1 Sep 8, 2022
@ntamas
Copy link
Member

ntamas commented Sep 11, 2022

Short enumeration:

  • implementation is used by Graph.Barabasi, Graph.pagerank, Graph.personalized_pagerank, Graph.induced_subgraph and Graph.community_spinglass
  • algorithm is used by Graph.eigen_adjacency
  • method is used by Graph.Degree_Sequence, Graph.Realize_Degree_Sequence, Graph.Tree_Game, Graph.feedback_arc_set, power_law_fit and compare_communities

Given the diversity of options, the effort needed to make them consistent (with proper deprecation cycles), the generic availability of tooltips in IDEs that always clarify which keyword arguments are supported by a function, the lack of large-scale complaints from users, and the possibility of a rewrite of the Python interface with generated interfaces, I'm putting this aside for the time being but keep the issue open. I'd rather not spend time on this for 0.10.1.

@ntamas ntamas removed this from the 0.10.1 milestone Sep 11, 2022
@stale
Copy link

stale bot commented Nov 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 12, 2022
@ntamas ntamas added todo Triaged for implementation in some unspecified future version and removed stale labels Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
todo Triaged for implementation in some unspecified future version
Projects
None yet
Development

No branches or pull requests

2 participants