Fix boolean slot featurization #6601
Conversation
| ("9", [1, 1]), | ||
| (12, [1, 1]), | ||
| ("9", [1, 0]), | ||
| (12, [1, 0]), |
There was a problem hiding this comment.
@tmbo I don't know why this was supposed to be 1.
JEM-Mosig
requested review from
alwx,
degiz,
federicotdn,
ricwo and
wochinge
as code owners
|
Ups... I clicked on "ready to review" and assumed that would tag @tmbo , but it added half a dozen people! Sorry 😬 |
changelog/6601.bugfix.md
Outdated
| Fixed a bug in the featurization of the boolean slot type. Previously, to set a slot value to "true", | ||
| you had to set it to "1", which is in conflict with the documentation. |
There was a problem hiding this comment.
You didn't have to set it to "1", just to true (no quotes)
- slot{"my_bool_slot": true}
There was a problem hiding this comment.
Ok, but that's still confusing users. I'll change the doc string.
There was a problem hiding this comment.
I guess I make this an improvement then, instead of a bug fix? Also, I am not sure about the float(x) == 1.0. Perhaps it'd be better to have float(x) != 0.0 as before, though that's a bit odd I think.
There was a problem hiding this comment.
Oh I totally agree that this is a good improvement! (or bugfix if you expected "true" to work). Just wanted to point out that saying you had to set it to 1 would probably confuse a lot of people whose bots were working as expected before.
There was a problem hiding this comment.
@erohmensing just FYI that while just true might work for training time, I think there might be an issue with the JSON validation schema since I got this error message when changing stories to use just true.
| @@ -36,7 +36,7 @@ mapping: | |||
| - type: "map" | |||
| mapping: | |||
| regex;(.*): | |||
| type: "text" | |||
| type: "any" | |||
There was a problem hiding this comment.
@erohmensing This fixes the yaml problem and true without quotes works again. But I have no idea if this is the proper way of doing this.
|
@JEM-Mosig |
@degiz I wasn't sure about this change. Strings and booleans should definitely be allowed. Also lists, since we have a list-type slot. And since you can define your own slot type, really anything should be allowed I guess... |
|
I'd rather allow |
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)