Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

add micro to pearsonr #16878

Merged
merged 9 commits into from
Dec 9, 2019
Merged

add micro to pearsonr #16878

merged 9 commits into from
Dec 9, 2019

Conversation

zburning
Copy link
Contributor

Description

add micro to pearson correlation coefficient.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@zburning zburning requested a review from szha as a code owner November 21, 2019 08:45
sxjscience
sxjscience reviewed Nov 21, 2019
View reviewed changes
python/mxnet/metric.py Outdated Show resolved Hide resolved
@sxjscience
Copy link
Member

Nice add! Could you also add the test here? https://github.com/apache/incubator-mxnet/blob/a8b31a239f5d5ed0ebff0f3be44b5e5534e0b3f5/tests/python/unittest/test_metric.py#L265-L272

Also, I suggest test against the scipy version: https://docs.scipy.org/doc/scipy/reference/generated/scipy.stats.pearsonr.html

@zburning
Copy link
Contributor Author

Actually, I also test the run time performance locally. But the current test_metric_perf.py doesn't test micro performance. Do you think it's necessary to add it? @sxjscience

leezu
leezu reviewed Nov 22, 2019
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! Please see two small comments above.

python/mxnet/metric.py Show resolved Hide resolved
python/mxnet/metric.py Show resolved Hide resolved
@leezu leezu added the R1.6.0 label Nov 26, 2019
@leezu
Copy link
Contributor

leezu commented Nov 26, 2019

@ptrendx this change may be good to include to 1.6. Do you have any concerns?

@sxjscience
Copy link
Member

@leezu For this one, we may not included it in R1.6.0 since it adds new functionality.

@leezu
Copy link
Contributor

leezu commented Nov 29, 2019

Current metrics is broken as it is inconsistent. Thus this functionality is a bugfix and should be included. There's no reason to leave it out.

@sxjscience
Copy link
Member

My concern is that it adds the average flag and is not a mere bug fix. @ptrendx will decide whether it is suitable to be included or not.

@leezu
Copy link
Contributor

leezu commented Nov 29, 2019

The current pearsonr metric is broken, because we cannot actually calculate the pearsonr but only an average of batch-wise pearsonrs. We should not ship broken features without need. That would be masochistic, deriving pleasure from our user's pain inflicted by the broken feature.
As this PR has very limited scope there should be no concern to include it from my perspective. But of course @ptrendx will take the final decision.

sxjscience
sxjscience reviewed Dec 5, 2019
View reviewed changes
python/mxnet/metric.py Outdated Show resolved Hide resolved
sxjscience
sxjscience reviewed Dec 5, 2019
View reviewed changes
python/mxnet/metric.py Outdated Show resolved Hide resolved
leezu
leezu reviewed Dec 6, 2019
View reviewed changes
python/mxnet/metric.py Outdated Show resolved Hide resolved
python/mxnet/metric.py Outdated Show resolved Hide resolved
leezu
leezu reviewed Dec 6, 2019
View reviewed changes
python/mxnet/metric.py Outdated Show resolved Hide resolved
python/mxnet/metric.py Outdated Show resolved Hide resolved
python/mxnet/metric.py Outdated Show resolved Hide resolved
@leezu leezu merged commit d58f6cb into apache:master Dec 9, 2019
ptrendx pushed a commit to ptrendx/mxnet that referenced this pull request Dec 9, 2019
@zburning @ptrendx
Add micro averaging strategy to pearsonr metric (apache#16878)
fd0ea16 
        Strategy to be used for aggregating across mini-batches.
            "macro": average the pearsonr scores for each batch.
            "micro": compute a single pearsonr score across all batches.
@ptrendx ptrendx mentioned this pull request Dec 9, 2019
Merged
ptrendx added a commit that referenced this pull request Dec 10, 2019
@ptrendx
Backport #16895, #16922, #16878, #16979 and #16900 to 1.6 (#17029)
c973f01 
* Fix ndarray indexing bug (#16895)

* Fix indexing bug

* More test cases

* Add test from 16647

* [Gluon] Update contrib.Estimator LoggingHandler to support logging per batch interval (#16922)

* Update LoggingHandler to support logging per interval

* Fix the constant variable issue in the logging handler

* Remove the constant variable hack in the logging handler.

* 1) replace LOG_PER_BATCH with LOG_PER_INTERVAL 2) add test case

* Improve the test script for LoggingHandler

* small fix on the test script

* logging handler test case bug fix

* remove parameter verbose from LoggingHandler

* move log_interval to the first argument

* resolve unittest mistakes

* Add micro averaging strategy to pearsonr metric (#16878)

        Strategy to be used for aggregating across mini-batches.
            "macro": average the pearsonr scores for each batch.
            "micro": compute a single pearsonr score across all batches.

* [Bugfix] [Numpy] Add `kAddTo` and kNullOp to Transpose (#16979)

* update

Check for repeated axes

enable addto to transpose

fix

fix

fix

fix

remove unused ndim

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

fix

Update pseudo2DTranspose_op-inl.cuh

try to fix

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

fix

Update np_matrix_op.cc

Update test_numpy_op.py

update test case

fix implementation

fix bug

update

fix bug

Update pseudo2DTranspose_op-inl.cuh

fix

fix

Update test_numpy_op.py

* Fix bug

* fix docstring

* try to address comment

* no need to change this line

* Fix bug

* address comments

* address comment

* introduce  gradient update handler to the  base estimator (#16900)

* introduce  gradient update handler to the  base estimator

* Modify the gradient update handler to include the batch size

* Remove unrelated gradient update handler.

* Modify gradient update handler to take the current batch size.

* Remove white space to avoid the sanity check failure

* add small tweak to the handler code

* Modify the documentation of priority parameter of relevant handlers.

* small modification on the documentation.

* Add small modification on the documentation.

* Remove unnecessary list check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sxjscience sxjscience sxjscience approved these changes

@leezu leezu leezu approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
R1.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zburning @sxjscience @leezu