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.

Sign up for GitHub

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

Jump to bottom

mcscript #1152

Merged
merged 6 commits into from
Oct 22, 2020
Merged

mcscript #1152

merged 6 commits into from
Oct 22, 2020

Conversation

wh629
Copy link
Contributor

@wh629 wh629 commented Oct 13, 2020

I'm having issues running black. I spoke with Jason and am submitting the PR without black.

Changes tested with the attached .sh and .sbatch files. Achieved 86.5% dev accuracy and found 86.6% from https://www.aclweb.org/anthology/D19-6012.pdf.

mcscript.zip

@pep8speaks
Copy link

pep8speaks commented Oct 13, 2020 •
edited
Loading

Hello @wh629! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 You can repair most issues by installing black and running: black -l 100 ./*. If you contribute often, have a look at the 'Contributing' section of the README for instructions on doing this automatically.

Comment last updated at 2020-10-21 21:36:23 UTC

@codecov
Copy link

codecov bot commented Oct 13, 2020 •
edited
Loading

Codecov Report

Merging #1152 into master will increase coverage by 0.04%.
The diff coverage is 69.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1152      +/-   ##
==========================================
+ Coverage   57.39%   57.44%   +0.04%     
==========================================
  Files         137      138       +1     
  Lines        9819     9861      +42     
==========================================
+ Hits         5636     5665      +29     
- Misses       4183     4196      +13
Impacted Files Coverage Δ
jiant/tasks/evaluate/core.py 37.15% <ø> (ø)
jiant/tasks/lib/mcscript.py 68.29% <68.29%> (ø)
jiant/tasks/retrieval.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da7550d...70f42f7. Read the comment docs.

jeswan
jeswan reviewed Oct 14, 2020
View reviewed changes
jiant/tasks/lib/mcscript.py Outdated Show resolved Hide resolved
@jeswan jeswan mentioned this pull request Oct 14, 2020
Merged
zphang
zphang requested changes Oct 16, 2020
View reviewed changes
Copy link
Collaborator

@zphang zphang left a comment

Choose a reason for hiding this comment

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

Removing the task property method, and add to
https://github.com/nyu-mll/jiant-dev/blob/master/md_docs/tasks.md

@zphang
Copy link
Collaborator

zphang commented Oct 16, 2020

And merge from master.

@wh629
Copy link
Contributor Author

wh629 commented Oct 18, 2020

@SimonOst

@zphang zphang merged commit d1b14c1 into nyu-mll:master Oct 22, 2020
leo-liuzy pushed a commit to leo-liuzy/dynamic_jiant that referenced this pull request Nov 11, 2020
@wh629 @jeswan
mcscript (nyu-mll#1152)
fe75e87 
* mcscript

* black

* add mcscript to documentation

* removed task property method

Co-authored-by: jeswan <57466294+jeswan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeswan jeswan jeswan approved these changes

@zphang zphang zphang approved these changes

@HaokunLiu HaokunLiu Awaiting requested review from HaokunLiu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wh629 @pep8speaks @zphang @jeswan