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

Further improvement of parameter operator test implementation #2631

Merged

Conversation

heplesser
Copy link
Contributor

This PR makes the parameter operator tests even more readable and efficient. It is a follow-up to #2628.

Concerning efficiency, tests using Py 3.11 indicate that using hasattr() instead of try-except improves performance by almost half.

One could argue that the test code here would become even simpler of the nest Parameter objects supported direct casting to int or float. But these parameters are only to be evaluated inside the C++ kernel, so adding __int__() or __float()__ methods at the Python level would only simplify test-writing, with no bearing on actual use. But writing code for the sake of testing only seems not sensible.

@heplesser heplesser added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Mar 12, 2023
@heplesser
Copy link
Contributor Author

I dug deeper into the xfail documentation and found that one can mark individual cases explicitly as xfail, while cases that should pass can be provided as usual. I hope you like the new implementation.

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.

This looks very nice! I'm guessing the newly added .bak file is unintentional and should therefore be removed before merge.

@heplesser
Copy link
Contributor Author

This looks very nice! I'm guessing the newly added .bak file is unintentional and should therefore be removed before merge.

Thanks for noticing, I have removed the file again.

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.

Just a small typo. Otherwise nice work!

testsuite/pytests/test_parameter_operators.py Outdated Show resolved Hide resolved
Co-authored-by: Jochen Martin Eppler <[email protected]>
@jougs jougs changed the title Further improvement parameter operator test implementation Further improvement of parameter operator test implementation Mar 14, 2023
@jougs jougs merged commit af912b8 into nest:master Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants