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

Refactoring test methods #8624

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

guilherme-mendes
Copy link
Contributor

Proposed changes:

  • Apply clean code and unused imports test_actions.py
  • Implements uncouple behaviour from test_count_vectors_featurizer

@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @alopez will take a look at it as soon as possible ✨

@sara-tagger sara-tagger requested a review from alopez May 7, 2021 06:00
Copy link
Contributor

@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.

Hi @guilherme-mendes. Thanks for cleaning this up! I just have a few minor comments.

tests/nlu/utilities.py Outdated Show resolved Hide resolved
tests/nlu/utilities.py Outdated Show resolved Hide resolved
@guilherme-mendes
Copy link
Contributor Author

Thanks for the feedback @JEM-Mosig, changes made.

@guilherme-mendes
Copy link
Contributor Author

Done! @JEM-Mosig @alopez

@JEM-Mosig
Copy link
Contributor

Sorry, @guilherme-mendes I didn't see the message. I typically only see this when you click the re-review arrows and request another review. I'll have a look today :)

@JEM-Mosig JEM-Mosig self-requested a review June 17, 2021 09:02
Copy link
Contributor

@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.

Almost there. One more thing to do.
Unfortunately I cannot merge main into your repo, so you'll have to do that before we can merge.

@@ -11,6 +11,14 @@ def write_file_config(file_config):
return f


# check if os sequences e sentences is loaded correctly
def get_feature_vectors(sequence, sentence):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add type annotations for the arguments and return types. Also, our convention is to put descriptions into the function's doc string, i.e.

def get_feature_vectors(sequence: ???, sentence: ???) -> Tuple[Optional[???], Optional[???]]:
  """Retrieves feature vectors from the features.
  
  Args:
    sequence: Features of individual tokens.
    sentence: Features of the entire string.
  
  Returns:
    Feature vectors (or None) for sequence and sentence features.
  """
  ...

You'll have to substitute the right thing for the question marks (just follow the return type of test_message1.get_sparse_features). Sorry I didn't see this earlier.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ guilherme-mendes
❌ itallogravina
You have signed the CLA already but the status is still pending? Let us recheck it.

@losterloh
Copy link
Contributor

@guilherme-mendes are you still intending to work on this? 🙂 if yes, please rebase onto main or merge main into your branch and see about all authors signing the CLA, then we're happy to review & approve it!

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

6 participants