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

Bug nlu comparison nlg responses rebase #5280

Closed
wants to merge 4 commits into from

Conversation

indam23
Copy link
Contributor

@indam23 indam23 commented Feb 20, 2020

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@indam23 indam23 requested a review from dakshvar22 February 20, 2020 15:02
@indam23
Copy link
Contributor Author

indam23 commented Feb 20, 2020

Due to the mess that ensued when trying to get #5202 up to date with 1.7.x to handle dependency issues in travis, this PR replaces that one

@degiz degiz added this to the Rasa 1.8 milestone Feb 20, 2020
@@ -0,0 +1,132 @@
## intent:affirm
Copy link
Member

Choose a reason for hiding this comment

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

do we really need to add a new example? does none of the existing ones work?

@@ -0,0 +1,9 @@
Fix bug encountering:
Copy link
Member

Choose a reason for hiding this comment

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

This will mess with the log rendering, the first line should be short. e.g.

Fixed incorrectly raised Error encountered in pipelines with a ``ResponseSelector`` and NLG.

...

@@ -0,0 +1,7 @@
## out of scope non english
Copy link
Member

Choose a reason for hiding this comment

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

same, I am sure we already have an example like this

@@ -734,6 +738,15 @@ def test_nlu_comparison(tmpdir):
run_1_path = os.path.join(output, "run_1")
assert set(os.listdir(run_1_path)) == {"50%_exclusion", "80%_exclusion", "test.md"}

exclude_50_path = os.path.join(run_1_path, "50%_exclusion")
modelnames = [os.path.basename(c)[:-4] for c in configs]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
modelnames = [os.path.basename(c)[:-4] for c in configs]
modelnames = [os.path.splitext(os.path.basename(config))[0] for config in configs]

@tmbo
Copy link
Member

tmbo commented Feb 21, 2020

Mhm did I miss something or is there no fix included for the error? as far as I can see you only changed test files

@indam23
Copy link
Contributor Author

indam23 commented Feb 21, 2020

@tmbo I must have left something out during the rebase-cherry pick. I need to move this to 1.8 anyway so will try again. sorry for the confusion.

@indam23
Copy link
Contributor Author

indam23 commented Feb 21, 2020

Should have been rebased on master

@indam23 indam23 closed this Feb 21, 2020
@indam23 indam23 deleted the bug_nlu_comparison_nlg_responses_rebase branch February 21, 2020 17:03
@tmbo tmbo mentioned this pull request Feb 24, 2020
4 tasks
@indam23
Copy link
Contributor Author

indam23 commented Feb 24, 2020

@tmbo I've made the changes requested on the other PR - sorry for missing these when I made that one.

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.

3 participants