search

caleb531 / automata

A Python library for simulating finite automata, pushdown automata, and Turing machines
https://caleb531.github.io/automata/
MIT License
349 stars 64 forks source link

Successors length limits #224

Closed Tagl closed 4 months ago

Tagl commented 5 months ago

Closes #207

Might be wise to add a few more tests.

coveralls commented 5 months ago

Coverage Status

coverage: 99.612%. remained the same when pulling aaaabfb0cf966cc3acf2301903b00c2170726177 on Tagl:successors_length_limits into 530016a03703e071b72f1665769770039018536d on caleb531:main.

coveralls commented 5 months ago

Coverage Status

coverage: 99.612%. remained the same when pulling 4183c714050e7e9826301e7431a0d9421464df4b on Tagl:successors_length_limits into 530016a03703e071b72f1665769770039018536d on caleb531:main.

eliotwrobson commented 5 months ago

Looks good from a glance! When I make a pass through the active PR list a little later I can add some tests before merging.

caleb531 commented 5 months ago

@eliotwrobson @Tagl Looks like a great addition to me! Thank you for this!

I am converting the PR to Draft state until more tests are added.

eliotwrobson commented 4 months ago

@Tagl added a couple of test cases and encountered a bug I commented on:

https://github.com/caleb531/automata/pull/224/commits/f95b0d17852cce7e5e8239fae83481d9d7f0da92#diff-404c46da3c65463b61109992f94cfd102f9f20f5467f1916bf9bf50ff89c7176R1894-R1895

I think this can be avoided just by popping from the char stack if the size goes over the length limit.

eliotwrobson commented 4 months ago

@Tagl for this, should the stop condition be changed to when the algorithm hits the string of all the lexicographically last character of length max_length? I think that might fix the issue I found in my testing.

Edit: just a heads up that we'll probably be closing up the PyOpenSci review soon, and it would be awesome if the release after that could include the changes here.

Tagl commented 4 months ago

I'll take a look tomorrow

Tagl commented 4 months ago

Should be fixed now, I added one more edge case to test for as well.

Tagl commented 4 months ago

Yes, I believe so.