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

Convert multimeter test from SLI to Python implementation #2652

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

heplesser
Copy link
Contributor

This PR provides a polished version of the Python implementation of the multimeter test ported from SLI.

@heplesser heplesser added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Mar 23, 2023
@heplesser
Copy link
Contributor Author

I am not sure why the Python tests fail here. It is again the numpy datatype problem, it seems, but that was fixed with #2571 and this PR does not touch that code. Also, all tests pass on my branch (https://github.com/heplesser/nest-simulator/actions/runs/4505551444) and locally on my machine.

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.

@heplesser To me it looks like it is the static code check that fails? See comments. I also made a suggestion for how we can obtain models with certain properties.

testsuite/pytests/test_multimeter.py Outdated Show resolved Hide resolved
testsuite/pytests/test_multimeter.py Outdated Show resolved Hide resolved
testsuite/pytests/test_multimeter.py Show resolved Hide resolved
testsuite/pytests/test_multimeter.py Outdated Show resolved Hide resolved
testsuite/pytests/test_multimeter.py Outdated Show resolved Hide resolved
testsuite/pytests/test_multimeter.py Outdated Show resolved Hide resolved
testsuite/pytests/test_multimeter.py Show resolved Hide resolved
@heplesser
Copy link
Contributor Author

@nicolossus The static checks now pass, we now install an up-to-date pycodestyle via pip; previously, we took an old version from a system location, thus the trouble with the :=.

I don't think the models() function an improvement in this case so I have not adopted it yet.

@nicolossus
Copy link
Member

nicolossus commented Mar 26, 2023

@heplesser I see, thanks for adding an up-to-date installation.

I agree that the models() function (with a better function name) would fit better in a helper library. However, I still think that the if-statement

if ('recordables' in (dflts := nest.GetDefaults(model)) and dflts['recordables'])

should be changed to (in my opinion) the more concise and readable

if nest.GetDefaults(model).get('recordables')

(Note dict.get(key) will return None if key is not in dict, which is in contrast to dict[key] that will raise KeyError)

@heplesser
Copy link
Contributor Author

@heplesser I see, thanks for adding an up-to-date installation.

I agree that the models() function (with a better function name) would fit better in a helper library. However, I still think that the if-statement ...

You are right, I didn't realize get() could be used like that. Changed it now, much more readable indeed.

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.

Thanks! LGTM.

Co-authored-by: Nicolai Haug <39106781+nicolossus@users.noreply.github.com>
@heplesser heplesser removed the request for review from jougs April 11, 2023 13:49
@heplesser heplesser merged commit 4f8dfc8 into nest:master Apr 11, 2023
6 checks passed
@heplesser heplesser deleted the move_mm_test_to_py branch September 15, 2023 18:08
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: Maintenance Work to keep up the quality of the code and documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants