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

Improve parameter test and implementation #2628

Merged
merged 8 commits into from
Mar 10, 2023
Merged

Improve parameter test and implementation #2628

merged 8 commits into from
Mar 10, 2023

Conversation

heplesser
Copy link
Contributor

As a step towards developing good test styles, this PR provides a re-write of test_parameter.py using PyTest features. It extends the scope of the tests and includes minor fixes:

  • float / Parameter is now supported
  • when attempting ** with Parameter as exponent, a TypeError is raised to conform with usual Python practice

@heplesser heplesser added T: Enhancement New functionality, model or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Mar 7, 2023
@heplesser heplesser added this to In progress in PyNEST-NG via automation Mar 7, 2023
nicolossus
nicolossus approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

testsuite/pytests/test_parameter.py Outdated Show resolved Hide resolved
jougs
jougs reviewed Mar 7, 2023
View reviewed changes
testsuite/pytests/test_parameter.py Outdated Show resolved Hide resolved
testsuite/pytests/test_parameter.py Outdated Show resolved Hide resolved
@heplesser heplesser mentioned this pull request Mar 7, 2023
Open
jougs
jougs reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

For me, the parametrize lines are really hard to read. I have added a suggestion to make them easier.

testsuite/pytests/test_parameter_operators.py Outdated Show resolved Hide resolved
heplesser and others added 3 commits March 8, 2023 11:23
@heplesser @jougs
Make code more readable.
e923f5f 
Co-authored-by: Jochen Martin Eppler <jougs@gmx.net>
@heplesser
Improve readability
f42f948
@heplesser
Changed opr to ops
d71c82c
@heplesser
Copy link
Contributor Author

@jougs I have now applied your suggestion to the remaining tests as well. I changed import operator as op to ... as ops to avoid having the same name for the module with all operands and for the local variable representing the individual operation inside each test.

jougs
jougs approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is very nice now.

@heplesser
Copy link
Contributor Author

@jougs Could you resolve the conversations so we can merge?

@jougs jougs merged commit 2ab766f into nest:master Mar 10, 2023
PyNEST-NG automation moved this from In progress to Done Mar 10, 2023
@heplesser heplesser mentioned this pull request Mar 12, 2023
Merged
@heplesser heplesser mentioned this pull request Mar 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jougs jougs jougs approved these changes

@nicolossus nicolossus nicolossus approved these changes

Assignees
No one assigned
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
PyNEST-NG
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@heplesser @jougs @nicolossus