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

Dyn 5427 confidence threshold autocomplete ml #13562

Merged
merged 12 commits into from
Nov 23, 2022
Merged

Dyn 5427 confidence threshold autocomplete ml #13562

merged 12 commits into from
Nov 23, 2022

Conversation

jesusalvino
Copy link
Contributor

Purpose

Implementation of the story https://jira.autodesk.com/browse/DYN-5427

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@QilongTang

FYIs

@RobertGlobant20 @filipeotero

@jesusalvino
Copy link
Contributor Author

nodeautocomplete

@QilongTang QilongTang added this to the 2.17.0 milestone Nov 22, 2022
@reddyashish
Copy link
Contributor

LGTM. The ML autocomplete request here https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Search/NodeAutoCompleteSearchViewModel.cs#L38 should be using these values from settings. Can you make that change as well and also resolve the conflicts.

@jesusalvino
Copy link
Contributor Author

LGTM. The ML autocomplete request here https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Search/NodeAutoCompleteSearchViewModel.cs#L38 should be using these values from settings. Can you make that change as well and also resolve the conflicts.

Done.

@reddyashish
Copy link
Contributor

Looks good to me with one comment. The only test failing is the IfSubmenuOpenOnMouseHover which is flaky.

Copy link
Contributor

@zeusongit zeusongit left a comment

Choose a reason for hiding this comment

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

LGTM with one comment

@jesusalvino
Copy link
Contributor Author

Looks good to me with one comment. The only test failing is the IfSubmenuOpenOnMouseHover which is flaky.

that test works for me locally
imagen

@reddyashish
Copy link
Contributor

reddyashish commented Nov 23, 2022

Yes that test was failing intermittently on master-5. We can ignore that.

@reddyashish
Copy link
Contributor

The last job has this failing test [Dynamo.Tests.Configuration.PreferenceSettingsTests.TestImportCopySettings]
Can you check it and lets run the job again,

@jesusalvino
Copy link
Contributor Author

The last job has this failing test [Dynamo.Tests.Configuration.PreferenceSettingsTests.TestImportCopySettings] Can you check it and lets run the job again,

Done.
imagen

@reddyashish
Copy link
Contributor

reddyashish commented Nov 23, 2022

Thanks @jesusalvino. We can merge this after the job gets finished. Once @QilongTang confirms if this can still get in for 2.17, we can cherry-pick it.

@reddyashish reddyashish merged commit b98b3d7 into DynamoDS:master Nov 23, 2022
QilongTang pushed a commit that referenced this pull request Nov 24, 2022
* Hide Nodes

* Update UI with Expander

* Node Autocomplete Section Done

* Set default number of results as 10

* restoring version

* restoring entire file

* Connect the confidence level and nuber of result settings

* Fix

* Labels

* Naming changes

* Updating the Settings properties for copy test
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