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

Add np.linalg.norm implementation #1251

Merged
merged 6 commits into from
Sep 18, 2023

Conversation

arjavtrivedi
Copy link
Contributor

@arjavtrivedi arjavtrivedi commented Mar 1, 2021

Ready for review:

  • Closes Missing np.linalg.norm implementation #1250
  • Executed pre-commit run --all-files with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/ as appropriate
  • Added an entry to the CHANGES file

Doc entry in docs/numpy.ipynb already covered by print statement here:

from pint.numpy_func import HANDLED_FUNCTIONS
print(sorted(list(HANDLED_FUNCTIONS)))

Copy link
Contributor

@keewis keewis left a comment

Choose a reason for hiding this comment

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

looks good to me, but someone with the permissions would have to rerun the tests so we can investigate the failures (the logs have expired since then).

pint/testsuite/test_issues.py Outdated Show resolved Hide resolved
@arjavtrivedi
Copy link
Contributor Author

arjavtrivedi commented Sep 14, 2023

I've managed to come back to this PR after quite a while. As such, I've updated the PR by merging upstream changes and addressed the previous feedback. The actions on the forked repository (here) appear to be passing, but I'm otherwise not sure why the PR workflows here are failing.

@AdVetter
Copy link

We are using Pint and np.linalg.norm a lot in our calculations and had to write helper a function to use it properly. Looking really forward to when this is merged! :-)

@keewis
Copy link
Contributor

keewis commented Sep 17, 2023

@arjavtrivedi (or @hgrecco / @jules-ch), could you merge in master? That should get RTD to build, and we'd need a maintainer to once again approve the workflow runs. Once tests pass I think this should be ready.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 17, 2023

CodSpeed Performance Report

Merging #1251 will improve performances by 20.16%

Comparing arjavtrivedi:feature/np-linalg-norm (cc56b12) with master (2852f36)

Summary

⚡ 1 improvements
✅ 420 untouched benchmarks

Benchmarks breakdown

Benchmark master arjavtrivedi:feature/np-linalg-norm Change
test_op2[eq-keys21] 161.6 µs 134.5 µs +20.16%

@arjavtrivedi
Copy link
Contributor Author

@arjavtrivedi (or @hgrecco / @jules-ch), could you merge in master? That should get RTD to build, and we'd need a maintainer to once again approve the workflow runs. Once tests pass I think this should be ready.

@keewis
Cheers - unfortunately I don't have write access else I'd merge!

@hgrecco hgrecco merged commit 247512b into hgrecco:master Sep 18, 2023
33 checks passed
@hgrecco
Copy link
Owner

hgrecco commented Sep 18, 2023

Thanks @arjavtrivedi !!

@arjavtrivedi
Copy link
Contributor Author

Thank you @hgrecco for the great software!

@keewis
Copy link
Contributor

keewis commented Sep 18, 2023

unfortunately I don't have write access else I'd merge!

This is not important anymore since the PR was merged (but I guess is good to know for any future PRs). Still, what I meant was to merge master into this branch (arjavtrivedi:feature/np-linalg-norm) to make use of some changes in master that fixed RTD.

@hgrecco
Copy link
Owner

hgrecco commented Sep 18, 2023

@keewis I think that the current master has everything ok, right?

@keewis
Copy link
Contributor

keewis commented Sep 18, 2023

yep, it was just that on this PR RTD failed because the configuration still contained the system_packages: true line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing np.linalg.norm implementation
4 participants