Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

[BUGFIX] Change sklearn accuracy metric to mxnet accuracy metric #1209

Merged
merged 3 commits into from
May 5, 2020
Merged

[BUGFIX] Change sklearn accuracy metric to mxnet accuracy metric #1209

merged 3 commits into from
May 5, 2020

Conversation

avinashsai
Copy link
Member

partially fixes #886

@avinashsai avinashsai requested a review from a team as a code owner April 24, 2020 16:09
@mli
Copy link
Member

mli commented Apr 24, 2020

Job PR-1209/2 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1209/2/index.html

@codecov
Copy link

codecov bot commented Apr 24, 2020 •
edited
Loading

Codecov Report

Merging #1209 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1209   +/-   ##
=======================================
  Coverage   87.42%   87.42%           
=======================================
  Files          81       81           
  Lines        7346     7346           
=======================================
  Hits         6422     6422           
  Misses        924      924

@avinashsai
Copy link
Member Author

avinashsai commented Apr 24, 2020 •
edited
Loading

@leezu
There is some issue with notedown. Output from notedown has some indendation issues. Should I fix them manually?

chenw23
chenw23 suggested changes Apr 25, 2020
View reviewed changes
Copy link
Member

@chenw23 chenw23 left a comment

Choose a reason for hiding this comment

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

And do you think there is other indentation problem in the output?

docs/examples/sentiment_analysis/self_attentive_sentence_embedding.md Outdated
processing (NLP) has still been effectively improved in many ways. Along with
the widespread use of embedding techniques, many other methods have been
developed to further express the semantics and meanings of sentences with words:
1. A vector representation of multiple words in a sentence can be concatenated
Copy link
Member

Choose a reason for hiding this comment

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

Would you please add a blank line before 1. to fix the indentation problem?
Thanks!

@avinashsai
Copy link
Member Author

@StrayBird-ATSH I guess there is a little discrepancy in the notebook presented on the website and the tutorial in the master branch. (https://gluon-nlp.mxnet.io/examples/sentence_embedding/self_attentive_sentence_embedding.html)

For example: heading in the master branch starts with" Training Structured Self-attentive Sentence Embedding", whereas on the website it is "A Structured Self-attentive Sentence Embedding".

@chenw23
Copy link
Member

chenw23 commented Apr 25, 2020

@StrayBird-ATSH I guess there is a little discrepancy in the notebook presented on the website and the tutorial in the master branch. (https://gluon-nlp.mxnet.io/examples/sentence_embedding/self_attentive_sentence_embedding.html)

For example: heading in the master branch starts with" Training Structured Self-attentive Sentence Embedding", whereas on the website it is "A Structured Self-attentive Sentence Embedding".

Hello Avinash,
The main website displays contents from our default branch, also the release branch (currently v0.9.x). For master branch website, please refer to https://gluon-nlp.mxnet.io/master/index.html

@mli
Copy link
Member

mli commented Apr 25, 2020

Job PR-1209/3 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1209/3/index.html

@avinashsai
Copy link
Member Author

@StrayBird-ATSH I have made few changes. please review this.

@avinashsai
Copy link
Member Author

@leezu can you review this?

@avinashsai
Copy link
Member Author

@StrayBird-ATSH Please review this.

@leezu
Copy link
Contributor

leezu commented Apr 29, 2020 •
edited
Loading

@avinashsai why is the diff so large? Are you developing on Windows and introducing different line ending characters for example? Or did the file previously use the Windows Line ending?

@avinashsai
Copy link
Member Author

I think it's about notedown output command.

  1. jupyter nbconvert notebook.ipynb --to markdown produces the output similar to current tutorial.

  2. Whereas notedown input.md > output.ipynb produces output with a different indentation.

I ran 2nd command first as mentioned in gluonnlp website. However difference was very large and indentation is not proper. So I ran 1st command and it produced output similar to current tutorial.
That's why the difference is large.

@leezu
Copy link
Contributor

leezu commented Apr 29, 2020

Many lines are unchanged but do show up as changed. Could you fix the PR so that you only change the actually intended to be changed lines?
Did you verify there is no problem with respect to incompatible Windows line ending characters? You can check autocrfl here https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration

@avinashsai
Copy link
Member Author

Fixed PR. Now it clearly shows the changes

@mli
Copy link
Member

mli commented Apr 30, 2020

Job PR-1209/4 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1209/4/index.html

@avinashsai
Copy link
Member Author

Iam not sure why the 2 checks failed

@chenw23
Copy link
Member

chenw23 commented May 1, 2020

Iam not sure why the 2 checks failed

I have restarted the CI for you. There has planned to be an update on CI to reduce the rate of unexpected failures.

@avinashsai
Copy link
Member Author

All checks have passed. Can you review this?

@chenw23
Copy link
Member

chenw23 commented May 1, 2020

All checks have passed. Can you review this?

Great! @leezu Please review this. Thanks!

@avinashsai
Copy link
Member Author

@leezu please review this

leezu
leezu approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@leezu leezu left a comment

Choose a reason for hiding this comment

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

Thank you @avinashsai

@leezu leezu merged commit 16b8cbd into dmlc:master May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chenw23 chenw23 chenw23 requested changes

@leezu leezu leezu approved these changes

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
@avinashsai @mli @chenw23 @leezu