Skip to content

fixed ParseTextField postprocessing, added genre field to MultiNLI #386

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

Merged
merged 5 commits into from
Sep 18, 2018

Conversation

putama
Copy link
Contributor

@putama putama commented Sep 17, 2018

  • Fix ParsedTextField postprocessing that still accepts three args instead of two
  • Added an option for ParsedTextField to whether to reverse the sentence as postprocessing
  • Include parse trees for testing and reverse postprocessing test

… reverse ParseTextField, added genre field multinli
@mttk
Copy link
Contributor

mttk commented Sep 17, 2018

@putama could you please fix the flake errors reported w.r.t. some line lengths and (just for my sake) elaborate on what was the error with the postprocessing fix?

@putama
Copy link
Contributor Author

putama commented Sep 17, 2018

Hi. I found that on field.py line 290, they changed the call to postprocessing function into only 2 arguments. This was the quickest fix I could think of.

@mttk
Copy link
Contributor

mttk commented Sep 17, 2018

Ok, cool. Could you just fix the last flake complaint (IMO, just if/else the function call and leave the lambdas as arguments)

@putama
Copy link
Contributor Author

putama commented Sep 18, 2018

Done, thanks. Could you tell what the latest unsuccessful check was about?

@mttk
Copy link
Contributor

mttk commented Sep 18, 2018

Functions are preferred to lambdas in PEP8 due to easier stack tracing (https://stackoverflow.com/questions/25010167/e731-do-not-assign-a-lambda-expression-use-a-def).
They seem reasonable here as it's unlikely to use them again so the hack around that check is to put them as default args in function calls, where they're allowed.

Thanks a lot for the fix!

@mttk mttk merged commit 13f5e3b into pytorch:master Sep 18, 2018
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.

2 participants