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

[WIP] Iterators for path states #221

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

Conversation

henrik-wolf
Copy link
Contributor

As indicated in issue #217, I started working on an AbstractPathIterator, to loop over all paths encoded in an AbstractPathState. Currently, only one subtype, FloydWarshallIterator is implemented and tested. I would very much like your feedback on the approach taken here, so that I can start implementing something similar for the other PathStates, if possible.
Here is a short list of the thinks I have done:

  • Added AbstractPathIterator as supertype to all path iterators (It felt like a very big decision to make the AbstractPathState or subtypes themselves iterable. I feel that would make sense. What do you think?)
  • Added FloydWarshallIterator as a wrapper around the AbstractPathState. Now, if you want to iterate over all paths, you need to somehow awkwardly wrap your state in a FloydWarshallIterator(state).
  • Added iterate methods needed for iteration
  • Added enumerate_path_into! method, which writes a path into a pre-allocated array and returns a view, dramatically cutting down on allocations, compared to enumerate_paths
  • added size, length and collect for this iterator. collect returns a matrix that works in the same way as the matrices in the state, that is, matrix[s,d] is the path from s to d. (this feels more logical, than the current output from enumerate_paths)
  • While I was at it, I reworked enumerate_paths a bit to cut down on allocations in general, but also to make a request for a path with given start and destination not do the extra work of calculating all paths from start and then returning only the one to destination

While I am happy with the changes to enumerate_paths, I would very much like your feedback on the decisions I took with the iterator part.

@henrik-wolf henrik-wolf marked this pull request as draft February 2, 2023 18:14
@codecov
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

Merging #221 (36bcc17) into master (c4360cf) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   97.43%   97.44%   +0.01%     
==========================================
  Files         113      113              
  Lines        6554     6583      +29     
==========================================
+ Hits         6386     6415      +29     
  Misses        168      168              

@gdalle gdalle added the enhancement New feature or request label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants