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

Standardize numpy imports with as np #1776

Closed
wants to merge 7 commits into from

Conversation

NeuralNoble
Copy link

@NeuralNoble NeuralNoble commented Jul 12, 2024

What is the change?

  • Replaced occurrences of "numpy" with "np" throughout the codebase.
  • Updated import statements to use "import numpy as np".

Why is the change being made?

  • Efficiency: Using "np" reduces typing effort and standardizes usage across the project.
  • Consistency: Aligns with common practice in Python programming for NumPy aliasing.
  • Readability: Improves code readability by shortening repetitive references to NumPy.

Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify any new/changed code. (Note: Please ensure relevant tests are added/updated as per the project requirements.)
  • The code style follows good practices.
  • The commit message(s) follow good practices.
  • The release notes have been updated if necessary. (Note: Update release notes if changes affect functionality or user experience.)
  • The documentation is still up-to-date in the doc folder. (Note: Verify and update documentation as needed.)
  • The dependencies are still up-to-date in pyproject.toml.

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2024

CLA assistant check
All committers have signed the CLA.

@jakehader jakehader linked an issue Jul 12, 2024 that may be closed by this pull request
@jakehader
Copy link
Member

Hi @NeuralNoble,

Thanks for contributing. Can you sign the CLA agreement before we run tests? It looks like there also may be a mismatch in your account email addresses so you'll probably need to fix that before you try to sign.

armi/bookkeeping/db/compareDB3.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/database3.py Outdated Show resolved Hide resolved
@jakehader jakehader changed the title Replaced "numpy" with "np" and updated import statements to "import numpy as np" Standardize numpy imports with as np Jul 12, 2024
@jakehader
Copy link
Member

jakehader commented Jul 12, 2024

Thanks @NeuralNoble - you may still need to add your other email address to your account for the CLA if you're using a separate email than your GitHub account on your personal machine. I believe this is configured on your git config locally.

Also, the black formatter and linting checks are failing so please run black and ruff on your branch. You can check the errors on the CI reports.

@john-science john-science self-requested a review July 12, 2024 15:18
@john-science john-science added cleanup Code/comment cleanup: Low Priority low priority Style points and non-features labels Jul 12, 2024
@NeuralNoble
Copy link
Author

While running ruff check as per your suggestion, I encountered an error regarding an undefined name (version) in our setup.py file. I have checked that all tests pass successfully with pytest, indicating that functionality-wise everything is working fine.
doc/tutorials/armi-example-app/setup.py:32:13: F821 Undefined name `__version__` Found 1 error.

Could you please provide guidance on how to resolve this linting error properly? It seems related to the version placeholder used for package versioning in setup.py. Should I define version differently or handle it in a specific way within the setup script?

@john-science
Copy link
Member

Please sign the CLA.

We will not be able to merge a PR without the CLA being signed.

@NeuralNoble
Copy link
Author

I have signed the CLA but the previous commit made by me isnt showing signed yet . do you want me to make a new pull-request?

@jakehader
Copy link
Member

I have signed the CLA but the previous commit made by me isnt showing signed yet . do you want me to make a new pull-request?

We have seen this issue a couple times. You may try to open a new PR to see if that fixes it, but @john-science would be best to provide a recommendation here I think.

It looks like the linting and black formatting tests pass now. Note that it may not be merged right away since we have other processes to check on as well. Appreciate the support on this issue.

@NeuralNoble
Copy link
Author

@jakehader thanks for the help and patience i am sorry if i kept bugging this is my first time contributing to open source so getting difficulty .

@john-science
Copy link
Member

@NeuralNoble As long as you sign the CLA once, I'm happy. I find that system to be a touch flaky.

@john-science
Copy link
Member

@NeuralNoble We have a log jam of a dozen PRs I will be shepherding through the pipeline next week. So this will probably take a few days to merge.

I'm just trying to care for our downstream user base. It'll get merged though, Not to worry.

armi/conftest.py Outdated Show resolved Hide resolved
armi/conftest.py Outdated Show resolved Hide resolved
@john-science
Copy link
Member

@NeuralNoble I believe the problem is that ARMI had commits into main that use the word "numpy" in a test file that you changed the import on.

  File "/Users/runner/work/armi/armi/armi/bookkeeping/db/tests/test_jaggedArray.py", line 60, in test_backwardsCompatible
    flattenedArray = numpy.array([1, 2, 3, 4, 5, 6, 7, 8, 9])

@john-science
Copy link
Member

@NeuralNoble I would fix this for you, but you are in a forked repo, so I would have to make a PR into your fork. Seems silly.

@NeuralNoble
Copy link
Author

@NeuralNoble I would fix this for you, but you are in a forked repo, so I would have to make a PR into your fork. Seems silly.

okay

@john-science
Copy link
Member

@NeuralNoble If you can merge main into your feature branch, I'd happily merge it.

@NeuralNoble
Copy link
Author

NeuralNoble commented Jul 18, 2024

@john-science my main is one commit behind the main of this repo so first i should sync the fork then merge my feature-branch?

@john-science
Copy link
Member

@john-science my main is one commit behind the main of this repo so first i should sync the fork then merge my feature-branch?

Yup!

Hit this button on your fork's home page:

image

Then do on the command line do:

cd armi
git checkout main
git pull origin main
git checkout numpy-np
git pull origin numpy-np
git merge main
# fix any conflicts, if they exist
git push origin numpy-np

@NeuralNoble
Copy link
Author

done

@john-science
Copy link
Member

@NeuralNoble The CI and linting in this branch are broke.

Listen, maybe it would be easier to just check out main fresh, redo the work to change the import, and force-push to your branch?

Just thinking aloud. The branch has been broke for a while now.

@john-science
Copy link
Member

This PR is suffering from time rot.

It would probably be easier to just do the numpy import replacement from scratch, and force-push the branch.

Up to you though.

@john-science
Copy link
Member

Okay, it was weeks, so I just solved this in a different PR: #1837

Sorry! Thanks for the offer to contribute!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority low priority Style points and non-features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to "import numpy as np"
4 participants