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

Simplifying get_useful_states with move iterator #353

Merged
merged 7 commits into from
Oct 3, 2023

Conversation

kilohsakul
Copy link
Collaborator

@kilohsakul kilohsakul commented Sep 28, 2023

Look at this to see an ultimate demonstration of the coolness of the davids move iterator.

Although, I am wandering whether it can be used anywhere else then here. I did not come with a good usage in the intersection for instance, because I wanted to get the vectors ot targets with the same symbol and then merge them. Maybe it is just my fault, but maybe we actually need something a bit different, I don't know.
For instance, iterator through symbol_post that skips over symbols where there are no targets ...
Just thinking.

@kilohsakul
Copy link
Collaborator Author

Just a straightforward use of the move iterator, should be no brainer.

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8684c78) 71.59% compared to head (d92fe19) 71.52%.
Report is 1 commits behind head on devel.

Additional details and impacted files
@@            Coverage Diff             @@
##            devel     #353      +/-   ##
==========================================
- Coverage   71.59%   71.52%   -0.07%     
==========================================
  Files          30       30              
  Lines        3622     3603      -19     
  Branches      835      830       -5     
==========================================
- Hits         2593     2577      -16     
+ Misses        737      736       -1     
+ Partials      292      290       -2     
Files Coverage Δ
include/mata/nfa/delta.hh 90.41% <100.00%> (-0.13%) ⬇️
src/nfa/nfa.cc 80.09% <88.88%> (-0.25%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@vhavlena vhavlena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Collaborator

@Adda0 Adda0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the suggested, the use of iterators is elegant and readable. We will see how many uses the iterators have. Different variants can be added as needed, if we find them useful.

src/nfa/nfa.cc Outdated Show resolved Hide resolved
@Adda0 Adda0 force-pushed the get_useful_states-with-move-iterator branch from a0fe73a to d92fe19 Compare October 3, 2023 11:26
@Adda0
Copy link
Collaborator

Adda0 commented Oct 3, 2023

It seems to be just a tad bit slower (due to creating the instances of the iterators), but the difference should not be that significant. The experiments will tell.

@Adda0 Adda0 merged commit 74a2c24 into devel Oct 3, 2023
@Adda0 Adda0 deleted the get_useful_states-with-move-iterator branch October 3, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants