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

Numpy add numpy op hanning, hamming, blackman#15815

Merged
reminisce merged 1 commit intoapache:masterfrom
gyshi:hanning
Sep 22, 2019
Merged

Numpy add numpy op hanning, hamming, blackman#15815
reminisce merged 1 commit intoapache:masterfrom
gyshi:hanning

Conversation

@gyshi
Copy link
Contributor

@gyshi gyshi commented Aug 9, 2019

Description

(Brief description on what this PR is about)

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 http://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

@gyshi gyshi requested a review from szha as a code owner August 9, 2019 05:36
@gyshi gyshi force-pushed the hanning branch 2 times, most recently from 9ecd328 to 14fc78f Compare August 9, 2019 05:51
@reminisce reminisce added the Numpy label Aug 9, 2019
@gyshi gyshi closed this Aug 9, 2019
@gyshi gyshi reopened this Aug 9, 2019
@gyshi gyshi closed this Aug 9, 2019
@gyshi gyshi reopened this Aug 9, 2019
haojin2
haojin2 reviewed Aug 15, 2019
View reviewed changes
@gyshi gyshi changed the title Numpy add numpy op hanning Numpy add numpy op hanning, hamming, blackman Aug 19, 2019
@gyshi gyshi force-pushed the hanning branch 4 times, most recently from 6cd146e to bea9be8 Compare September 2, 2019 12:52
comaniac
comaniac suggested changes Sep 11, 2019
View reviewed changes
Copy link

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Please rebase to the master and solve conflicts after resolving comments.

python/mxnet/ndarray/numpy/_op.py Outdated


@set_module('mxnet.ndarray.numpy')
def hanning(M, dtype=_np.float64, ctx=None):
Copy link

@comaniac comaniac Sep 11, 2019

Choose a reason for hiding this comment

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

The default dtype is float32.

comaniac
comaniac suggested changes Sep 14, 2019
View reviewed changes
python/mxnet/ndarray/numpy/_op.py Outdated


@set_module('mxnet.ndarray.numpy')
def hanning(M, dtype=None, ctx=None):
Copy link

@comaniac comaniac Sep 14, 2019

Choose a reason for hiding this comment

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

I still prefer to use float64 as the default type in the function argument instead of having a checker at the beginning of the function. The reason is that the type of dtype would be Union[None, str, numpy.dtype] in the current implementation, which is inconsistent with the docstring saying that dtype : str or numpy.dtype. Also, it makes the semantic ambiguous and should be avoided especially in Python 3.7. If the default value of dtype in the argument is np.float64 or 'float64' then we can enforce its type to be Union[str, numpy.dtype] as documented.

Copy link
Contributor Author

@gyshi gyshi Sep 15, 2019

Choose a reason for hiding this comment

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

it's good, i think i should use float32 as the defaulet type,

@gyshi gyshi force-pushed the hanning branch 2 times, most recently from c1ac152 to d5754e4 Compare September 15, 2019 01:36
comaniac
comaniac approved these changes Sep 15, 2019
View reviewed changes
Copy link

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. I don't have further comments.

haojin2
haojin2 approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@haojin2 haojin2 left a comment

Choose a reason for hiding this comment

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

LGTM

@gyshi
add numpy op hanning, hamming, blackman
32213d2 
address test file

fix test file

make default dtype is float32
@reminisce reminisce merged commit 53ebe12 into apache:master Sep 22, 2019
drivanov pushed a commit to drivanov/incubator-mxnet that referenced this pull request Sep 26, 2019
@gyshi @drivanov
add numpy op hanning, hamming, blackman (apache#15815)
cba4335 
address test file

fix test file

make default dtype is float32
larroy pushed a commit to larroy/mxnet that referenced this pull request Sep 28, 2019
@gyshi @larroy
add numpy op hanning, hamming, blackman (apache#15815)
ce948fe 
address test file

fix test file

make default dtype is float32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

2 more reviewers

@comaniac comaniac comaniac approved these changes

@haojin2 haojin2 haojin2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Numpy

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gyshi @comaniac @haojin2 @reminisce