Skip to content

Support complex relative grammar #987

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

Merged
merged 42 commits into from
Oct 7, 2022
Merged

Support complex relative grammar #987

merged 42 commits into from
Oct 7, 2022

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Oct 2, 2022

Partial #13
Fixes #979

image

Checklist

  • Update relative modifier grammar
  • Add new keywords to csv files
  • Update cheat sheet
  • Update docs
  • I have added tests

@AndreasArvidsson AndreasArvidsson linked an issue Oct 2, 2022 that may be closed by this pull request
16 tasks
@AndreasArvidsson
Copy link
Member Author

The grammar is now updated to support every thing from the image in issue #13.

image

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Cool!! 😍

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

One thing that's missing in this PR from the diagram is "second last" (equivalent to "second to last" in English)

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

  • Oh yeah we should also do a quick DFA test before merge just in case

@AndreasArvidsson
Copy link
Member Author

@pokey I have updated the legend extension side, but I'm not sure how to actually test it. That cheat sheet doesn't work in development host for me.

Since I know that you are going to rewrite all of the documentation any how do you mind just doing it originally the way you want it?

@AndreasArvidsson
Copy link
Member Author

Yeah but that's only the first time isn't it? Switching back and forth will use the cache?

@pokey
Copy link
Member

pokey commented Oct 6, 2022

Yes, but it's still not too hard to get a cache miss when I was testing. Idk maybe let's drive this PR for a day to see how painful it is

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Oct 6, 2022

Sounds good. I was really hoping that we wouldn't have to bother with dfa performance anymore :/

btw for this pr switching to ts: 2022-10-06 16:15:23 IO [audio]=1200.000ms [compile]=1701.832ms [emit]=33.019ms [decode]=5.512ms [total]=1740.363ms

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Oct 6, 2022

@pokey I found an optimization for number_small. Basically instead of using an advance capture with multiple terms I just turned it into a single list. I made a few different performance measurements and it's very clear that a list is definitely the fastest.

@ctx.capture("number_small", rule=f"({alt_digits} | {alt_teens} | {alt_tens} [{alt_digits}])") # 1710ms
@ctx.capture("number_small", rule=f"one") # 710ms
@ctx.capture("number_small", rule="{user.number_small}") # 667ms
@ctx.capture("number_small", rule=f"{'|'.join(number_small)}") # 1794ms
def number_small(m): 
    return int(parse_number(list(m)))

The commit for my repository. If you can't find any flaw we should probably upstream this to Knausj and dfa will be good enough.
AndreasArvidsson/andreas-talon@452aad5

If we want to optimize further there is probably multiple captures in Cursorless that we could turn into lists if we wanted to.

@pokey
Copy link
Member

pokey commented Oct 7, 2022

oh yeah good call. yeah like you suggest we could probably inline a bunch of compound captures into big lists. certainly for things that don't include customisable spoken forms. for lists that are customisable we could just regenerate the lists if part of it changes. but yeah I'll try out the number_small optimisation and see if that gets us to a good enough spot

@pokey
Copy link
Member

pokey commented Oct 7, 2022

Wow just tested locally. Absolute game changer. Filed PR on knausj to save you the trouble talonhub/community#993. I'll finish reviewing this PR and I think we can get this one in and just tell people they need to update their knausj once that PR is merged

@pokey
Copy link
Member

pokey commented Oct 7, 2022

Just tried it on my personal fork, and it's <600ms, so that's good. Still slower than I'm used to, but good enough for now.

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Oct 7, 2022

Nice. I saved over a second in compile time on this pr. That is quite massive for such a small change :)
Now I think it's good enough to merge, but I think we should probably try to figure out some more optimizations.

pokey
pokey previously approved these changes Oct 7, 2022
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Ok I'm happy with it, but I made a bunch of changes, so have a look. I changed all the capture formatting in cheatsheet; see screenshot in description

@pokey
Copy link
Member

pokey commented Oct 7, 2022

Actually one more thing let me drop diagram into docs

@pokey
Copy link
Member

pokey commented Oct 7, 2022

Ok good to go; have a look to make sure it all seems good to you and then feel free to merge if so

@AndreasArvidsson AndreasArvidsson merged commit eef8bfc into main Oct 7, 2022
@AndreasArvidsson AndreasArvidsson deleted the ordinal_grammar branch October 7, 2022 18:58
cursorless-bot pushed a commit that referenced this pull request Oct 7, 2022
* Added support for plural lists

* Updated relative scope modifier grammar

* Added ordinals to relative modifier

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Added plural list for regex scopes

* Update cursorless-talon/src/modifiers/relative_scope.py

Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

* Update cursorless-talon/src/modifiers/relative_scope.py

Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

* Added link to inflection license

* Use plural scope type in ordinal scope

* Added support for <ordinal> last scope grammar

* Clean up

* Added words to csv

* Updated modifier grammar

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Updated legend

* Added docs

* Update defaults.json

* Start tweaking modifier cheatsheet

* More cheatsheet updates

* More tweaks

* Cleanup cheatsheet representation

* Some talon cleanup

* Change formatting for cheatsheet captures

* Fix captures in cheatsheet

* More capture cleanup

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Revert ts config

* Revert old cheatsheet changes

* Fixes

* Tweak docs

* More doc tweaks

* Add diagram

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this pull request Mar 27, 2024
* Added support for plural lists

* Updated relative scope modifier grammar

* Added ordinals to relative modifier

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Added plural list for regex scopes

* Update cursorless-talon/src/modifiers/relative_scope.py

Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

* Update cursorless-talon/src/modifiers/relative_scope.py

Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

* Added link to inflection license

* Use plural scope type in ordinal scope

* Added support for <ordinal> last scope grammar

* Clean up

* Added words to csv

* Updated modifier grammar

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Updated legend

* Added docs

* Update defaults.json

* Start tweaking modifier cheatsheet

* More cheatsheet updates

* More tweaks

* Cleanup cheatsheet representation

* Some talon cleanup

* Change formatting for cheatsheet captures

* Fix captures in cheatsheet

* More capture cleanup

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Revert ts config

* Revert old cheatsheet changes

* Fixes

* Tweak docs

* More doc tweaks

* Add diagram

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
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.

Add ordinal scope types Add ordinal terms to overrides csv, docs and cheatsheet
3 participants