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.

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

fix: cross2d wrong doc. reference (issue #6276) #6277

Merged
merged 7 commits into from Sep 24, 2020
Merged

Conversation

jeertmans
Copy link
Contributor

@jeertmans jeertmans commented Sep 23, 2020

See issue #6276 for more details

Closes #6276

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. Think it'd be a good idea to update the unit tests too to make sure it's less likely to happen again. Suggest updating the import here to reflect the public API location:

from numba.np.arraymath import cross2d

and then checking the correct path is in the error string here:
self.assertIn(
'Dimensions for both inputs is 2',
str(raises.exception)
)

Thanks!!

CHANGE_LOG Outdated
@@ -1199,7 +1199,7 @@ Enhancements from user contributed PRs (with thanks!):
* Lucio Fernandez-Arjona extended Numba's ``np.sum`` support to now accept the
``dtype`` kwarg in #4472.
* Pedro A. Morales Maries added support for ``np.cross`` in #4128 and also added
the necessary extension ``numba.numpy_extensions.cross2d`` in #4595.
the necessary extension ``numba.np.extensions.cross2d`` in #4595.
Copy link
Contributor

Choose a reason for hiding this comment

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

As the change log is a reflection of history I don't think that this should be changed, irrespective of this being in the wrong place for current Numba.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok my bad, I was doubtful about changing this but thank you for the information

numba/np/arraymath.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added the 4 - Waiting on author Waiting for author to respond to review label Sep 23, 2020
@stuartarchibald stuartarchibald added this to the PR Backlog milestone Sep 23, 2020
@jeertmans
Copy link
Contributor Author

Yes indeed, thank you for noticing @stuartarchibald . I did update the code.

I ran the test and it seems ok:

(numbaenv) jerome@jerome-N501VW:~/Documents/programmation/numba$ python -m numba.runtests numba.tests.test_np_functions.TestNPFunctions.test_cross_exceptions
>>> .
>>> ----------------------------------------------------------------------
>>> Ran 1 test in 2.907s
>>> 
>>> OK

jeertmans and others added 4 commits September 23, 2020 18:15
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
as mentionned, it is preferable to keep changelog, even if it contained errors
@esc
Copy link
Member

esc commented Sep 24, 2020

@jeertmans thanks for addressing the issue comments. From what I understand, @stuartarchibald suggested to add a check for the correct error string to the unit test. Something like:

 self.assertIn( 
     "a call to `cross2d(a, b)` from `numba.np.extensions`", 
     str(raises.exception) 
 ) 

To double check that the path listed in the error message is indeed the correct one.

@jeertmans
Copy link
Contributor Author

Thanks for the explanation @esc ! I did commit an additional check for that purpose. I hope it follows your standards.

@esc
Copy link
Member

esc commented Sep 24, 2020

@jeertmans excellent, thank you very much! I'll mark it as Ready to Merge under the assumption that @stuartarchibald will agree.

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Sep 24, 2020
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch and fixes.

@stuartarchibald
Copy link
Contributor

@jeertmans Congratulations on your first contribution to Numba!

@jeertmans
Copy link
Contributor Author

Thank you both @stuartarchibald and @esc , I appreciate your help and I hope to be able to contribute more :)

@sklam sklam merged commit 38ecdf9 into numba:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

numba advices to use cross2d but specified path is wrong
4 participants