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

Number predicate #100

Merged
merged 22 commits into from
Dec 17, 2022
Merged

Number predicate #100

merged 22 commits into from
Dec 17, 2022

Conversation

kilohsakul
Copy link
Collaborator

The class for initial and final states, with constant test and update (sort of constant),
pruning the additions removals etc from the interface of NFA,
some other small changes.
(The class can be also used for tagging things in case the tags then must be reset,
one could e.g. use it in reachability, make the tags static, reset them after every run :). )
Some algoroithms such as complementation are not yet updated to work with final/initial states reasonably, for later.

@kilohsakul kilohsakul requested review from martinhruska, Adda0, tfiedor and ondrik and removed request for martinhruska and Adda0 December 6, 2022 15:01
@kilohsakul
Copy link
Collaborator Author

Ok, now there are at least no conflicts. There were conflicts because of moving things around and renaming files, I hope I did not mess something up.

I will need Lachtan to help with the python binding, pretty please.

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.

This is a first quick review of the introduced changes. The idea of introducing classes for initial and final states makes sense to me and I support it. Whether it works well for us, that remains to be seen. In general, In general, I have no serious issues with the PR, the mentioned problems are mostly of cosmetic character. I will do a proper review of the functionality of the introduced NumPredicate later.

include/mata/nfa.hh Outdated Show resolved Hide resolved
include/mata/nfa.hh Show resolved Hide resolved
include/mata/nfa.hh Show resolved Hide resolved
include/mata/nfa.hh Show resolved Hide resolved
README.md Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
@kilohsakul
Copy link
Collaborator Author

not sure I will have time to go through all comments, sorry

@Adda0
Copy link
Collaborator

Adda0 commented Dec 11, 2022

No worries. Someone else from us can resolve these when the Python binding gets fixed before merging the PR.

Copy link
Member

@tfiedor tfiedor left a comment

Choose a reason for hiding this comment

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

I like the idea, I think the code is fine, however, I would prefer if we discussed the naming around the elements part, because I feel it could be better. I proposed some initial suggestions, but I know we can do better.

Also delegate some minor fixes to some oompa loompa (martin, juraj, david).

include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Show resolved Hide resolved
include/mata/util.hh Outdated Show resolved Hide resolved
src/nfa/nfa.cc Show resolved Hide resolved
src/nfa/nfa.cc Show resolved Hide resolved
Copy link
Member

@martinhruska martinhruska left a comment

Choose a reason for hiding this comment

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

Once all notes are addressed I approve PR.

README.md Show resolved Hide resolved
include/mata/nfa.hh Show resolved Hide resolved
include/mata/nfa.hh Show resolved Hide resolved
include/mata/nfa.hh Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
include/mata/number-predicate.hh Show resolved Hide resolved
include/mata/number-predicate.hh Outdated Show resolved Hide resolved
src/tests-synchronized-iterator.cc Outdated Show resolved Hide resolved
@martinhruska
Copy link
Member

We need to progress this somehow. Lukas addressed some of your comments, please check them and eventually mark them as solved.

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.

Besides what was already mentioned above and what we have discussed. The introduced functionality looks good to me.

include/mata/number-predicate.hh Outdated Show resolved Hide resolved
@martinhruska
Copy link
Member

@tfiedor Could you please fix the binding?

@tfiedor
Copy link
Member

tfiedor commented Dec 16, 2022

@tfiedor Could you please fix the binding?

I will, once Mr. L commands me, that he is finished with his changes.

@kilohsakul
Copy link
Collaborator Author

merge it merge it!

@martinhruska
Copy link
Member

Based on request by Lukas, I merge this PR. Biding will be fixed later.

@martinhruska martinhruska merged commit 2a9678e into devel Dec 17, 2022
@martinhruska martinhruska deleted the NumberPredicate branch December 17, 2022 11:06
@tfiedor
Copy link
Member

tfiedor commented Dec 17, 2022

I think that was a bad call, is there a reason why we had to merge it right now?

@kilohsakul
Copy link
Collaborator Author

Yes, we need to speed this up, in order to be able to use the up-to-date mata in the string solver, we need to work intensively on the string solver now.

@kilohsakul
Copy link
Collaborator Author

Deadline close.

This was referenced Dec 18, 2022
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.

4 participants