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

nvar property returns length of state variables #550

Closed
wants to merge 1 commit into from

Conversation

geeythree
Copy link

@geeythree geeythree commented Apr 14, 2022

This PR resolves issue #549

Previously the Models could specify incorrect _nvar.
The nvar property is now updated and returns the length of state_variables.

Existing behavior:

class Model:
    _nvar = 3
    state_variables = 'r', 'V'

m = Model()
m.nvar()  != len(m.state_variables)

New behavior:

class Model:
    state_variables = 'r', 'V'

m = Model()
m.nvar()  == len(m.state_variables)

Previously the Models can specify incorrect _nvar. The nvar property is now updated and returns the length of state_variables
@geeythree
Copy link
Author

Hello, I am new to this community and would love to engage and contribute further. As I am still learning the TVB code base, I had chosen issue #549 to familiarise myself with the same. I request the maintainers to kindly go through the PR. 😃

@liadomide liadomide requested a review from maedoc April 14, 2022 16:14
@liadomide liadomide added the bug Something isn't working label Apr 14, 2022
@maedoc
Copy link
Member

maedoc commented Apr 14, 2022

Thanks for your contribution. Hope to see you on some of the other issues 👍

@maedoc
Copy link
Member

maedoc commented Apr 28, 2022

@geeythree can you additionally go through the different model classes and remove their custom _nvar defintions. For instance, this line can be removed.

Also note the rateML generated models currently generate an _nvar attribute which can also be removed.

@geeythree
Copy link
Author

@maedoc Sure, I can do that. Shall I update these changes in the same PR?

@maedoc
Copy link
Member

maedoc commented Apr 28, 2022

Yes, please just push more commits.

@maedoc
Copy link
Member

maedoc commented May 30, 2024

closing for #720 which has the rest of the changes requested

@maedoc maedoc closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scilib bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants