-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
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 |
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 |
There was a problem hiding this 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.
@ptrendx this change may be good to include to 1.6. Do you have any concerns? |
@leezu For this one, we may not included it in R1.6.0 since it adds new functionality. |
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. |
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. |
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. |
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.
* 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
Description
add micro to pearson correlation coefficient.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments