Fix epoch override for TEDPolicy #9182
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dakshvar22
requested review from
a team,
aeshky and
ka-bu
and removed request for
a team and
aeshky
2 tasks
ka-bu
suggested changes
Jul 21, 2021
| 20, | ||
| TEDPolicy.defaults[EPOCHS], | ||
| ), # trained_policy uses default epochs during training | ||
| (False, TEDPolicy.defaults[EPOCHS], TEDPolicy.defaults[EPOCHS]), |
There was a problem hiding this comment.
Suggested change
| (False, TEDPolicy.defaults[EPOCHS], TEDPolicy.defaults[EPOCHS]), | |
| (True, TEDPolicy.defaults[EPOCHS]+1, TEDPolicy.defaults[EPOCHS]+1), |
And why are the first two cases with "True" and 2/1 special?
There was a problem hiding this comment.
I wanted to have a test case with the default value of epoch_override in the load function, i.e. 1. I've now changed it to TEDPolicy.defaults[EPOCHS].
2 was just an arbitrary number other than the default but I've now changed it to TEDPolicy.defaults[EPOCHS] + 1
Do you think the extra test case with the default value of the function is unnecessary?
There was a problem hiding this comment.
- I think we could remove:
(False, TEDPolicy.defaults[EPOCHS], TEDPolicy.defaults[EPOCHS],),because I'm not sure what this would test and because(False, TEDPolicy.defaults[EPOCHS]+1, TEDPolicy.defaults[EPOCHS],),already tests that the override value is ignored. - And I think we could remove
(True, TEDPolicy.defaults[EPOCHS], TEDPolicy.defaults[EPOCHS]),because it would just test that the epoch is overwritten with the (identical) epoch_overwrite value (so it would also pass if the function did not override anything :D) while(True, TEDPolicy.defaults[EPOCHS] + 1, TEDPolicy.defaults[EPOCHS] + 1),tests exactly that, no? - Regarding the default values, I think that the above should suffice as test (we can't test all n \in N here and 1/2 are/should be no special cases really regarding the overwrite), no?
There was a problem hiding this comment.
You've convinced me. Removed 👍
ka-bu
approved these changes
Jul 22, 2021
2 tasks
aeshky
added a commit
that referenced
this pull request
Aug 6, 2021
* Add ability to point to a certificate in endpoints.yml (#9118) * Add cert file functionality to Endpoint * Apply suggestions from code review types Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> * update changelog Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> * Fix epoch override for TEDPolicy (#9182) * bump rasa-sdk dep and min compat version * fix bug add tests * add changelog * change test cases * remove test cases * prepared release of version 2.8.1 (#9183) * update rasa-sdk * prepared release of version 2.8.1 * update lock * Make UnexpecTEDIntentPolicy compatible with E2E data (#9203) * bump rasa-sdk dep and min compat version * add test and code * add changelog and refactor method * add test * more test cases * use applied_events * 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (#9237) * I removed the experimental feature designation from the docs. * I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example. * I removed the unused import: rasa.shared.utils.common * Adding change log files for issues 8791 and 8024. * Rewording change logs to make it clear that the behaviour of the features remains unchanged. * prepared release of version 2.8.2 (#9262) * prepared release of version 2.8.2 * Updated the date of release. * install yarn dependency in cloned repository, in docs publication workflow * silence yarn warning * Fix typo in push_docs_to_branch.sh * Generated a new poetry lock file Co-authored-by: Joe Juzl <joejuzl@gmail.com> Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> Co-authored-by: Daksh Varshneya <d.varshneya@rasa.com> Co-authored-by: m-vdb <m.verger@rasa.com>
ErickGiffoni
pushed a commit
to FGA-GCES/rasa
that referenced
this pull request
Aug 31, 2021
* Add ability to point to a certificate in endpoints.yml (RasaHQ#9118) * Add cert file functionality to Endpoint * Apply suggestions from code review types Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> * update changelog Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> * Fix epoch override for TEDPolicy (RasaHQ#9182) * bump rasa-sdk dep and min compat version * fix bug add tests * add changelog * change test cases * remove test cases * prepared release of version 2.8.1 (RasaHQ#9183) * update rasa-sdk * prepared release of version 2.8.1 * update lock * Make UnexpecTEDIntentPolicy compatible with E2E data (RasaHQ#9203) * bump rasa-sdk dep and min compat version * add test and code * add changelog and refactor method * add test * more test cases * use applied_events * 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (RasaHQ#9237) * I removed the experimental feature designation from the docs. * I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example. * I removed the unused import: rasa.shared.utils.common * Adding change log files for issues 8791 and 8024. * Rewording change logs to make it clear that the behaviour of the features remains unchanged. * prepared release of version 2.8.2 (RasaHQ#9262) * prepared release of version 2.8.2 * Updated the date of release. * install yarn dependency in cloned repository, in docs publication workflow * silence yarn warning * Fix typo in push_docs_to_branch.sh * Generated a new poetry lock file Co-authored-by: Joe Juzl <joejuzl@gmail.com> Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> Co-authored-by: Daksh Varshneya <d.varshneya@rasa.com> Co-authored-by: m-vdb <m.verger@rasa.com>
ErickGiffoni
pushed a commit
to FGA-GCES/rasa
that referenced
this pull request
Sep 9, 2021
* Add ability to point to a certificate in endpoints.yml (RasaHQ#9118) * Add cert file functionality to Endpoint * Apply suggestions from code review types Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> * update changelog Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> * Fix epoch override for TEDPolicy (RasaHQ#9182) * bump rasa-sdk dep and min compat version * fix bug add tests * add changelog * change test cases * remove test cases * prepared release of version 2.8.1 (RasaHQ#9183) * update rasa-sdk * prepared release of version 2.8.1 * update lock * Make UnexpecTEDIntentPolicy compatible with E2E data (RasaHQ#9203) * bump rasa-sdk dep and min compat version * add test and code * add changelog and refactor method * add test * more test cases * use applied_events * 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (RasaHQ#9237) * I removed the experimental feature designation from the docs. * I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example. * I removed the unused import: rasa.shared.utils.common * Adding change log files for issues 8791 and 8024. * Rewording change logs to make it clear that the behaviour of the features remains unchanged. * prepared release of version 2.8.2 (RasaHQ#9262) * prepared release of version 2.8.2 * Updated the date of release. * install yarn dependency in cloned repository, in docs publication workflow * silence yarn warning * Fix typo in push_docs_to_branch.sh * Generated a new poetry lock file Co-authored-by: Joe Juzl <joejuzl@gmail.com> Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com> Co-authored-by: Daksh Varshneya <d.varshneya@rasa.com> Co-authored-by: m-vdb <m.verger@rasa.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes:
epoch_overridewas being used even when policy was being loaded in non-finetune mode. Because of this wrong warnings get generated which confuse users.Status (please check what you already did):
black(please check Readme for instructions)