Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Possible Code Refactoring #2

Open
1 of 9 tasks
cczhu opened this issue Jun 17, 2019 · 3 comments
Open
1 of 9 tasks

Possible Code Refactoring #2

cczhu opened this issue Jun 17, 2019 · 3 comments

Comments

@cczhu
Copy link
Contributor

cczhu commented Jun 17, 2019

Transplanting comments by @candu on the other repo to here.

  • make digraph_to_bipartite always return a directed graph; calling methods can then use .to_undirected to return an undirected graph.
  • use map() to build the bipartite graph, e.g. nodesb_top = map(lambda item: str(item) + '_i', G.nodes()).
  • Refactor linker so that each linker algorithm is a class, inheriting from LinkerBase which houses the digraph_to_bipartite and matching_to_digraph routines. After this, we can also split up the matching_to_digraph class into subroutines to_directed_matching and directed_matching_to_digraph. This allows us to:
    • drop return_matching=True.
    • move check_degree et al. into the unit tests.
  • min_edge = c_edges[0]... -> return min(c_edges, key=lambda edge: Gw.edges[edge][mintype]) because Python has solved it for us.
  • self._disable_tqdm = True if not progress_bar else False -> self._disable_tqdm = not progress_bar
  • Consider refactoring get_paths to
remaining_nodes = set(G.nodes)
while remaining_nodes:
  current_path = []
  cnode = remaining_nodes.pop()
  # more magic...
  • Add fuzzy testing for get_paths.
@cczhu
Copy link
Contributor Author

cczhu commented Jun 27, 2019

Now that we have OR-Tools and agent modelling in the other repo:

  • Refactor batched linker to require users to manually specify a linking algorithm. This might be somewhat annoying for the user, but will force them to examine the API or documentation to see which algorithms are available. Internally, we could eliminate the batched linker subclasses, making the code much more elegant.
  • Similarly, we can force the user to pass a shift tracking object to the batched linker shift class.
  • Possibly generalize the shift tracker to handle all linking cases. get_paths should then return a list of path objects, not a list of values (it should still be trivial to return a list of lists, since that functionality is built into the ShiftTracker class already).
  • Set shift termination rules in each DriverShiftBase subclass, so that we only need one ShiftTracker class that the user needs to instantiate.

@cczhu
Copy link
Contributor Author

cczhu commented Aug 12, 2019

Will be each of these off to individual small PRs. Updating as needed.

@cczhu
Copy link
Contributor Author

cczhu commented Aug 23, 2019

"Refactor linker..." and "Refactor batched linker" comments partly addressed by #7.

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

No branches or pull requests

1 participant