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

Move radius, diameter and eccentricity methods from generic_graph.py to graph.py and digraph.py #29660

Closed
vipul79321 mannequin opened this issue May 7, 2020 · 13 comments

Comments

@vipul79321
Copy link
Mannequin

vipul79321 mannequin commented May 7, 2020

Currently radius, diameter and eccentricity computation methods are in generic_graph.py file. Since graph and digraph both have different algorithms for their computation. So it would be nice to have these method separately for graph and digraph in respective files.

CC: @dcoudert

Component: graph theory

Keywords: gsoc20

Author: Vipul Gupta

Branch/Commit: 9e2dcc8

Reviewer: David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/29660

@vipul79321 vipul79321 mannequin added this to the sage-9.2 milestone May 7, 2020
@vipul79321
Copy link
Mannequin Author

vipul79321 mannequin commented May 7, 2020

Branch: u/gh-vipul79321/ticket29660

@vipul79321 vipul79321 mannequin added the s: needs review label May 7, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

bf852a4radius, diameter, eccentricity, center, periphery moved to graph and digraph separately

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 7, 2020

Commit: bf852a4

@vipul79321
Copy link
Mannequin Author

vipul79321 mannequin commented May 10, 2020

Author: Vipul Gupta

@dcoudert
Copy link
Contributor

comment:4
  • There is no need to add before the methods
+    # Eccentricity
+    # Radius
  • This part is a perfect example of why we must add an eccentricity method in boost backend. It makes no sense to use boost to compute APSP, then return a dictionary of dictionaries with the distances (taking very long time to build and consuming a lot of memory) and finally get the largest value. We could directly return the eccentricities from boost.
+            if algorithm in ['Floyd-Warshall-Python', 'Floyd-Warshall-Cython', 'Johnson_Boost']:
+                dist_dict = self.shortest_path_all_pairs(by_weight, algorithm,
+                                                         weight_function,
+                                                         check_weight)[0]
+                algorithm = 'From_Dictionary'

Overall, I think this change ease the identification of missing methods and bad implementation choices. We must avoid as much as possible the use of dist_dict.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

9e2dcc8unneccessary comments removed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

Changed commit from bf852a4 to 9e2dcc8

@vipul79321
Copy link
Mannequin Author

vipul79321 mannequin commented May 12, 2020

comment:6

Replying to @dcoudert:

  • There is no need to add before the methods
+    # Eccentricity
+    # Radius

Done

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:7

LGTM.

@dcoudert
Copy link
Contributor

Changed keywords from gsoc to gsoc20

@dcoudert
Copy link
Contributor

comment:8

just update the keyword to gsoc20

@vbraun
Copy link
Member

vbraun commented Jun 3, 2020

Changed branch from u/gh-vipul79321/ticket29660 to 9e2dcc8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants