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.

Sign up for GitHub

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

Jump to bottom

Refactor methods without arguments into functions #685

Merged
merged 30 commits into from
Jul 18, 2023
Merged

Refactor methods without arguments into functions #685

merged 30 commits into from
Jul 18, 2023

Conversation

mathisrichter
Copy link
Contributor

@mathisrichter mathisrichter commented May 8, 2023 •
edited
Loading

Objective of pull request: The Loihi2 class with static property methods is producing critical linting errors. As I don't see a need for this to be a class, I am suggesting to refactor it into functions in a loihi2 module.

Before merging this change, all other Lava libraries using it will have to be fixed as well.
@mathisrichter will make those changes.

Pull request checklist

Your PR fulfills the following requirements:

  • Issue created that explains the change and why it's needed
  • Tests are part of the PR (for bug fixes / features)
  • Docs reviewed and added / updated if needed (for bug fixes / features)
  • PR conforms to Coding Conventions
  • PR applys BSD 3-clause or LGPL2.1+ Licenses to all code files
  • Lint (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
  • Build tests (pytest) passes locally

Pull request type

Please check your PR type:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe): API change due to linting fix

What is the current behavior?

from lava.utils.system import Loihi2
Loihi2.is_loihi2_available  # 'is_available' is a static property on a class, which linting does not like

What is the new behavior?

from lava.utils.system import loihi2
loihi2.is_available()

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

@mathisrichter mathisrichter self-assigned this May 8, 2023
tim-shea
tim-shea approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@tim-shea tim-shea left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for working to clean things up!

@PhilippPlank
Copy link
Contributor

I assume the internal unit tests have also been tested?

bamsumit
bamsumit approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@bamsumit bamsumit 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 Mathis

@mathisrichter
Added new set of functions by @tim-shea, refactored.
b2e48d5 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Fixed linter error: unused import.
4d50c28 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Fixed linter and security errors.
84fab83 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Fixed linter errors.
cf8ca15 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Trying to make nosec work.
c9ac3f1 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Moved nosec back; does not seem to make a difference.
252cf9c 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Unit tests for some of the new functions in the slurm module.
9c10d15 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Full unit tests for SLURM module.
b023a9e 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Full unit tests for lava_loihi module.
a31bc93 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Deactivated linter error.
e2ecf7d 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Trying different nosec variants.
0572a64 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Trying different nosec variants.
1e673de 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Trying different nosec variants.
57d4bed 
Signed-off-by: Mathis Richter <[email protected]>
@tim-shea tim-shea added this to the Lava v0.8 milestone May 15, 2023
@tim-shea
Copy link
Contributor

Given that this will be a breaking change to some lib code and some end user code, I've tagged with Lava v0.8 milestone.

@tim-shea tim-shea merged commit efb56bf into lava-nc:main Jul 18, 2023
6 checks passed
This was referenced Jul 18, 2023
Closed
Merged
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
@mathisrichter @tim-shea
Refactor methods without arguments into functions (lava-nc#685)
baed791 
* Refactoring methods without arguments into functions.

Signed-off-by: Mathis Richter <[email protected]>

* Reverting unintentional change to tutorial.

Signed-off-by: Mathis Richter <[email protected]>

* Added new set of functions by @tim-shea, refactored.

Signed-off-by: Mathis Richter <[email protected]>

* Fixed linter error: unused import.

Signed-off-by: Mathis Richter <[email protected]>

* Fixed linter and security errors.

Signed-off-by: Mathis Richter <[email protected]>

* Fixed linter errors.

Signed-off-by: Mathis Richter <[email protected]>

* Trying to make nosec work.

Signed-off-by: Mathis Richter <[email protected]>

* Moved nosec back; does not seem to make a difference.

Signed-off-by: Mathis Richter <[email protected]>

* Unit tests for some of the new functions in the slurm module.

Signed-off-by: Mathis Richter <[email protected]>

* Full unit tests for SLURM module.

Signed-off-by: Mathis Richter <[email protected]>

* Full unit tests for lava_loihi module.

Signed-off-by: Mathis Richter <[email protected]>

* Deactivated linter error.

Signed-off-by: Mathis Richter <[email protected]>

* Trying different nosec variants.

Signed-off-by: Mathis Richter <[email protected]>

* Trying different nosec variants.

Signed-off-by: Mathis Richter <[email protected]>

* Trying different nosec variants.

Signed-off-by: Mathis Richter <[email protected]>

* Redesigned the API for lava.utils.{slurm,loihi}.

Signed-off-by: Mathis Richter <[email protected]>

* Another attempt at fixing linting.

Signed-off-by: Mathis Richter <[email protected]>

* Trying with noqa

Signed-off-by: Mathis Richter <[email protected]>

* Fixed module error in test patch

Signed-off-by: Mathis Richter <[email protected]>

* Renamed is_lava_loihi_installed to is_installed.

Signed-off-by: Mathis Richter <[email protected]>

* Fixed code duplication of patch decorators.

Signed-off-by: Mathis Richter <[email protected]>

* Added doc string to patch decorator.

Signed-off-by: Mathis Richter <[email protected]>

* Finished loihi and slurm docstrings.

---------

Signed-off-by: Mathis Richter <[email protected]>
Co-authored-by: Timothy Shea <[email protected]>
Co-authored-by: Tim Shea <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-shea tim-shea tim-shea approved these changes

@PhilippPlank PhilippPlank PhilippPlank approved these changes

@mgkwill mgkwill Awaiting requested review from mgkwill

@weidel-p weidel-p Awaiting requested review from weidel-p

@bamsumit bamsumit Awaiting requested review from bamsumit

Assignees

@mathisrichter mathisrichter

Labels
None yet
Projects
None yet
Milestone
Lava v0.8
Development

Successfully merging this pull request may close these issues.
4 participants
@mathisrichter @PhilippPlank @tim-shea @bamsumit