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

Remove erroneous check in stimulation device base class #2172

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

sdiazpier
Copy link
Contributor

This PR solves issue 83 from EBRAINS-cosim. A control had been added by mistake to the stimulation devices during the integration of the current version of NEST I/O. This control did not allow changing the status of the device once it had been initialized.

@sdiazpier
Copy link
Contributor Author

Dear @jougs and @hakonsbm could you please take a look at this small fix for cosim?

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 for your contribution. The change in this PR looks good to me.

Given the potential for future problems, I wonder if we should add a test to testsuite/pytests that checks the expected behavior for all stimulation devices. Could you please comment on this?

@jougs jougs added I: External API Developers of extensions or other language bindings may need to adapt their code S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation labels Sep 28, 2021
@jougs jougs added this to To do in Models via automation Sep 28, 2021
@jougs jougs added this to the NEST 3.2 milestone Sep 28, 2021
@jougs jougs changed the title Removed set status control in stimulation device Remove erroneous check in stimulation device base class Sep 28, 2021
@sdiazpier
Copy link
Contributor Author

Dear @jougs thanks for taking a look at the PR. We can indeed try to design a test suite for stimulation devices. I think the normal functionality without the new IO backends is already quite well tested within other tests but there are some cosim specific features which might be important to keep in check.

@jougs
Copy link
Contributor

jougs commented Sep 29, 2021

Merging after an offline discussion with @sdiazpier. Tests will be added in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: External API Developers of extensions or other language bindings may need to adapt their code S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation
Projects
Models
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants