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

Refactor string solving operations #398

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

Adda0
Copy link
Collaborator

@Adda0 Adda0 commented Mar 8, 2024

This PR allows inserting word from states with levels different from 0. The level to start from is read from the source since the source must already exist in the NFT anyway. No need for an additional parameter.

Furthermore, the PR applies this improvement in some string operations. Further PRs to simplify the string operation functions will come in the future.

@Adda0 Adda0 requested a review from koniksedy March 8, 2024 08:35
@Adda0
Copy link
Collaborator Author

Adda0 commented Mar 8, 2024

@koniksedy Could you have a look at the changes to insert word and see whether all looks good?

@Adda0
Copy link
Collaborator Author

Adda0 commented Mar 8, 2024

Hmm, never mind. Something broke. Wait with the review until I resolve the issue, please.

src/nft/nft.cc Show resolved Hide resolved
src/nft/nft.cc Show resolved Hide resolved
@koniksedy
Copy link
Collaborator

Hmm, never mind. Something broke. Wait with the review until I resolve the issue, please.

Too late. 😄 Overall, it looks good to me. I only have minor comments.

@Adda0
Copy link
Collaborator Author

Adda0 commented Mar 8, 2024

The reason it broke is that the latest changes to insert_identity() allowed for symbol jump transitions. Previously we agreed that transducers will not have symbol jump transitions, so I implemented the whole of string operations on NFTs with this assumption. When operations start adding symbol jump transitions, everything breaks. Therefore, I will disable creating symbol jump transitions in create_identity() for now. The operations that we implement may support them, but no operation should create them for now. When there is time to refactor the string solving, These symbol jump transitions can be enabled again.

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.31%. Comparing base (5a94d88) to head (85d9cc8).
Report is 1 commits behind head on states_with_levels.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           states_with_levels     #398      +/-   ##
======================================================
- Coverage               74.51%   74.31%   -0.20%     
======================================================
  Files                      43       43              
  Lines                    5509     5474      -35     
  Branches                 1221     1213       -8     
======================================================
- Hits                     4105     4068      -37     
- Misses                    958      964       +6     
+ Partials                  446      442       -4     

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

@Adda0
Copy link
Collaborator Author

Adda0 commented Mar 8, 2024

What do you think about the temporary fix for symbol jump transitions?

@koniksedy
Copy link
Collaborator

The operations that we implement may support them, but no operation should create them for now.

Well, then I have bad news for you. The function nft::insert_levels() does the exact same thing. It uses jump transitions to simplify the transducer. Thought, this behavior will not show on transducers with up to 2 levels, so your implementation should be fine, we need to keep it in mind.

@Adda0
Copy link
Collaborator Author

Adda0 commented Mar 8, 2024

I noticed, but as you said, unless one is creating an identity from level 0 to level 0, it does not matter for string solving. I will make a note to resolve this in the future, however.

@koniksedy
Copy link
Collaborator

OK. We can merge the PR.

@koniksedy koniksedy merged commit ce57daf into states_with_levels Mar 8, 2024
18 checks passed
@koniksedy koniksedy deleted the refactor_string_solving_operations branch March 8, 2024 10:18
Adda0 pushed a commit that referenced this pull request Mar 28, 2024
Adda0 pushed a commit that referenced this pull request Apr 15, 2024
Adda0 pushed a commit that referenced this pull request Nov 18, 2024
Adda0 pushed a commit that referenced this pull request Nov 21, 2024
Adda0 pushed a commit that referenced this pull request Nov 21, 2024
Adda0 pushed a commit that referenced this pull request Nov 22, 2024
Adda0 pushed a commit that referenced this pull request Dec 2, 2024
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.

2 participants