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

initial prototype (refactoring of policy/tracker featrizer/single state featurizer) #9074

Closed
wants to merge 14 commits into from

Conversation

JEM-Mosig
Copy link
Contributor

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)

Copy link
Contributor Author

@JEM-Mosig JEM-Mosig left a comment

Choose a reason for hiding this comment

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

I had a first read-through. Its a good start I think!

Comment on lines 75 to 77
That is, this function filters all sparse sequence features and then
turns each of these sparse sequence features into sparse sentence feature
by summing over their first axis, respectively.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't always work. For example, the "natural" operation to combine semantic map embeddings is not a sum.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is something that depends on the kind of Features and their origin (which means this will not make separate state featurizer classes necessary), I think we'll leave this in here as TODO (for a future Feature rework or so?)

rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
Comment on lines 157 to 159
# 1. Would be cool if users would be notified that they're not training
# the policy based on what their NLU pipeline returns.... (cf.
# featurize_via_interpreter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

I was really confused whether the wrong interpreter might be used by accident. But I guess the reason there's no mismatch (in practice now) is in the .persist of the TrackerFeaturizer which will pickles this StateFeaturizer as a wholeand preserve the "use_regex..." flag.

rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
self, state: State, interpreter: NaturalLanguageInterpreter
) -> Dict[Text, List["Features"]]:
"""Encode the given state with the help of the given interpreter.
class EntityFeaturizer:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow. Why do you want to implement a separate class here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It just helped me understand that the functionality that follows is completely independent of the rest (at least the functions itself). The logic seems to overlap and might be merged/solved with some of the above. No?

Copy link
Contributor Author

@JEM-Mosig JEM-Mosig left a comment

Choose a reason for hiding this comment

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

Just looked through the state.py again

rasa/shared/core/state.py Show resolved Hide resolved
rasa/shared/core/state.py Show resolved Hide resolved
rasa/shared/core/state.py Show resolved Hide resolved
rasa/shared/core/state.py Show resolved Hide resolved
@ka-bu ka-bu closed this Aug 5, 2021
@ka-bu ka-bu changed the title wip (single state featurizer) initial prototype (refactoring of policy/tracker featrizer/single state featurizer) Aug 5, 2021
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.

None yet

2 participants