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

CUDASimulation::setEnvironmentProperty() #760

Merged
merged 1 commit into from
Jan 17, 2022
Merged

CUDASimulation::setEnvironmentProperty() #760

merged 1 commit into from
Jan 17, 2022

Conversation

Robadob
Copy link
Member

@Robadob Robadob commented Jan 14, 2022 •
edited
Loading

Adds CUDASimulation::setEnvironmentProperty(), CUDASimulation::getEnvironmentProperty(), similar to the existing setPopulationData(), getPopulationData(). Added one thorough test to C and SWIG suites respectively.

Other Changes:

  • EnvironmentManager.setProperty()/getProperty() did not check that length=1
  • The test HostEnvironment::ExceptionPropertyReadOnly was calling non-array method with array types.

Todo:

  • SWIG template expansion
  • Tests

@Robadob Robadob self-assigned this Jan 14, 2022
@Robadob Robadob requested a review from ptheywood January 17, 2022 14:41
@Robadob
Adds CUDASimulation::setEnvironmentProperty()/getEnvironmentProperty()
a7fc09e 
Additional Swig specific setEnvironmentPropertyArray()/get..
Thorough tests in both C and SWIG test suites.

Additional minor fixes to EnvironmentManager::setProperty()/getProperty(), they were not checking that property is not an array property.
Fixed the test HostEnvironment::ExceptionPropertyReadOnly which was exposed as incorrect by the prior bugfix.
@Robadob Robadob force-pushed the cudasim_getset_envprop branch from 421083e to a7fc09e Compare January 17, 2022 14:51
@Robadob Robadob marked this pull request as ready for review January 17, 2022 14:51
ptheywood
ptheywood approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

Diff looks good to me, test coverage looks reasonable too.

I've built the test suite, but only ran the CUDASimulation sub-suite, which all passes under linux.

@Robadob
Copy link
Member Author

Robadob commented Jan 17, 2022

I ran the full C suite, should be fine.

Will merge it now, as I want/need it for the primage thing i'm working on.

@Robadob Robadob merged commit 9d0774b into master Jan 17, 2022
@Robadob Robadob deleted the cudasim_getset_envprop branch January 17, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptheywood ptheywood ptheywood approved these changes

Assignees

@Robadob Robadob

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Robadob @ptheywood