- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Edge view for graphs #27408
Comments
Branch: public/27408_edgeview |
comment:1
This is a first draft of an Feedback is more than welcome, for instance concerning:
New commits:
|
Commit: |
comment:2
I think having this separate In Python there is a difference between an iterator (what you obtain with
|
comment:3
OK. We can still do We can certainly also replace I have also create a class |
comment:4
Replying to @dcoudert:
The various combinatorial sets in sage implements
To avoid slowdown there are several possibilities:
Then remove it for now. I think that there are enough troubles introducing this |
comment:5
Replying to @videlec:
Actually, the above makes sense if you want the following to be fast
(Am I right that |
comment:6
Actually, in the current implementation, when
I don't know how to do that...
Yes. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:8
Why the attributes The Why do you store the What I mean by cache is that the graph class could have a |
comment:9
You want this to be fast. I would certainly do this in Cython. |
comment:10
I think it should be called |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:12
Again for performance, this should be as low-level as possible so I would drop the inheritance from |
comment:13
Replying to @videlec:
I disagree here. I would agree with you if we would be adding a completely new feature. But here, we are changing an existing feature (namely |
comment:14
What do you think of this proposal from #22349?
|
comment:15
I'm not going to insist on the cythonization, but I'd like to hear the opinion from others on this ticket. |
comment:16
Replying to @dcoudert:
I would say yes, since the old |
comment:17
I also suggest to keep the interface as simple as possible. For example, for the I also wouldn't do To check for a |
comment:18
In various places, you are replacing calls to That looks like a backwards change: isn't the eventual goal to deprecate |
comment:19
Cython allows you to use if self._sort_edges:
yield from sorted(self, key=self._sort_edges_key)
elif self._graph._directed:
yield from self._graph._backend.iterator_out_edges(self._vertices, self._labels)
if self._ignore_direction:
yield from self._graph._backend.iterator_in_edges(self._vertices, self._labels)
else:
yield from self._graph._backend.iterator_edges(self._vertices, self._labels) which I find easier to read and it's probably more efficient too. |
comment:89
|
comment:90
The graph is modified inside the loop, so yes we need to wrap with list. |
comment:91
do you want to wait for another round of review by Jeroen ? if not, you can set to positive on my behalf |
comment:93
I had to fix a small issue induced by recent change in |
comment:94
Let's wait for the patchbot to see if everything is in order, but for me this patch is now good to go (and the html documentation displays well). |
Reviewer: Jeroen Demeyer, Vincent Delecroix, Frédéric Chapoton |
comment:95
I let you guys decide if another round of review is necessary or not. As this is a big change, shouldn't we add a specific note in the release changelog or in the documentation of the graph module ? |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:97
When reading this thread i was surprised not to be in cc of this ticket as it refers to an email of mine, now i realize that there is a confusion in names between Nicolas Thiéry and Thierry Monteil (it is not the first time). I had some plans to implement that, but i am even happier that someone did the job, i will have a look to the branch. |
comment:98
Any feedback is more than welcome, and if happy, you can set to positive. We will also have to work on vertices and neighbors views. |
comment:99
It would be good to have this feature inside 9.0. |
comment:100
ok, let it be. Vincent, si tu veux faire une objection, c'est le moment. |
Changed branch from public/27408_edgeview to |
How long it should take until the deprecation warning is removed? |
Following sagemath#22349 and sagemath#27408, we finally set parameter `sort` to `False` by default for methods `vertices` and `edges`, and remove the corresponding deprecation warnings. ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36073 Reported by: David Coudert Reviewer(s): Matthias Köppe
This ticket implements an edge view for graphs, as discussed in #22349 and in this message.
An edge view is a read-only iterable container of edges enabling operations like
for e in E
ande in E
. In can be iterated multiple times. Checking membership is done in constant time. It avoids the construction of edge lists and so consumes little memory.CC: @nthiery @sagetrac-tmonteil
Component: graph theory
Author: David Coudert
Branch/Commit:
3af5bba
Reviewer: Jeroen Demeyer, Vincent Delecroix, Frédéric Chapoton
Issue created by migration from https://trac.sagemath.org/ticket/27408
The text was updated successfully, but these errors were encountered: