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

Epicodim3 #70

Merged
merged 11 commits into from Jan 12, 2018
Merged

Epicodim3 #70

merged 11 commits into from Jan 12, 2018

Conversation

LenSpek
Copy link
Contributor

@LenSpek LenSpek commented Nov 9, 2017

My implementation of the Epileptor Codim 3 model by Marisa.
(Ignore the first 3 commits, they were added by accident)

A python implementation of Marisa's codim 3 Epileptor model for TVB
Fixed a bug in the ultra-slow modulation and improved some documentation
Adjusted the packages to be in line with the other TVB-models
High Codimension Singularity and the Ultra-slow Transitions of Classes.*
Journal of Mathematical Neuroscience. 2017;7:7. doi:10.1186/s13408-017-0050-8.

.. The Epileptor codim 3 model is a neural mass model which contains two subsystems acting at different timescales.
Copy link
Member

Choose a reason for hiding this comment

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

please reflow to 80 columns, in Vim this is gqip but your editor may be different. or I can do it for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will adjust this

self.F = self.F / numpy.linalg.norm(self.F)


class EpileptorCodim3_slowmod(Model):
Copy link
Member

Choose a reason for hiding this comment

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

Is this a completely different model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This model is similar, it is a different implementation. It allows for multiple classes of bursters to be visited by a simulation. The model dynamics are essentially the same. However there are some differences in the input of the system and some extra dynamics to allow the transition of classes

@maedoc
Copy link
Member

maedoc commented Nov 9, 2017

@LenSpek looking good; I have a few requests:

  • keep the dfuns as you have them but try to add numba versions, as in the epileptor 6d model, and use it as the dfun. please test & eventually commit to this branch/PR
  • please provide some reproductions of figures from the paper either as a Python script or prefereably as a Jupyter notebook

Reflowed the documentation to a column with of 80 and improved the documentation of the _slowmod implementation
Copy link
Member

@maedoc maedoc left a comment

Choose a reason for hiding this comment

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

Looking good! 👍🏼
Careful though, if you implement Numba dfun for second model, you should change the names to be unique .

@maedoc
Copy link
Member

maedoc commented Nov 15, 2017

How much time do you have left to work in this model, btw?

@LenSpek
Copy link
Contributor Author

LenSpek commented Jan 10, 2018

I have time till the end of January. However if there are a few minor things, I will be able to address these after my internship has ended

@maedoc maedoc merged commit 0eddbf9 into the-virtual-brain:trunk Jan 12, 2018
@maedoc
Copy link
Member

maedoc commented Jan 12, 2018

thanks and sorry this took so long.

@liadomide
Copy link
Member

After this recent merge, one unit-test fails:

tvb-library/tvb/tests/simulator/simulator_test.py: TestSimulator.test_simulator_region

Error Message
TypeError: ufunc '_numba_dfun' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
....
../scientific_library/tvb/simulator/models/epileptorcodim3.py:286: TypeError

Can you please check if the test was wrongly written, or the new model is missing some API implicit requirement ?

@maedoc
Copy link
Member

maedoc commented Jan 12, 2018

The test probably assumes all parameters are floats while some parameters in this model are integers, and it's unsafe to convert a float to an integer.

""""The dfun using numba for speed"""
state_variables_ = state_variables.reshape(state_variables.shape[:-1]).T
coupling_ = coupling.reshape(coupling.shape[:-1]).T
derivative = _numba_dfun(state_variables_, coupling_, self.E[0], self.E[1], self.E[2], self.F[0], self.F[1],
Copy link
Member

Choose a reason for hiding this comment

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

@LenSpek can you set a breakpoint here and double check that the types of those non-float parameters being passed to the numba dfun match or if not, cast their values correclty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double-checked and the variables in question (self.modification, self.N) have dtype int32.

Copy link
Member

Choose a reason for hiding this comment

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

is that the case for defaults as well (when you don't specify any model parameters)? this test runs your model as if you made a simulation without specifying any parameters, only defaults

@LenSpek
Copy link
Contributor Author

LenSpek commented Jan 18, 2018

I think that I found what was triggering the tests: Modification is specified by default as a BoolArray, which TVB converts to an int32. I will push a change to address this, so the test works.
(BTW, when I run the TVB tests, I don't get this error)

@liadomide
Copy link
Member

liadomide commented Jan 18, 2018

If i debug in my env, the 2 params which are expected to be int32 are in fact int64.
I get the same error both in unit-test and in the ipython notebook that came with these new models.
I suspect this is the default of the installation.

@LenSpek
Copy link
Contributor Author

LenSpek commented Jan 18, 2018

I find that strange as I use the TVB classes from array.py and there the class IntegerArray is defined as:

class IntegerArray(BaseArray):
    _ui_name = "Array of integers"
    dtype = basic.DType(default=numpy.int32)

@liadomide
Copy link
Member

Could we use dtype int_ ?
This would become int32 on Windows OS and int64 on Unix systems.

I tested on Anaconda x64 with Python 2.7.14 and numpy 1.13 on Linux, Mac and Windows. If we replace the current int32 with int_, then all 3 systems are ok. Otherwise, with the current code only Windows is fine, the others are not working.

@maedoc
Copy link
Member

maedoc commented Jan 19, 2018

@liadomide can you paste a git diff of your int_ change, so it's clear where the change is made?

@maedoc
Copy link
Member

maedoc commented Jan 19, 2018

Numba supports the dtypes in question, and I would prefer platform independence if possible. (int_ corresponds to C long)

@liadomide
Copy link
Member

diff.txt

@liadomide
Copy link
Member

int_ is the default Integer type for Numpy, that is why I suggest it.
Seems to work well with numba int_
https://docs.scipy.org/doc/numpy-1.13.0/user/basics.types.html

@maedoc
Copy link
Member

maedoc commented Jan 19, 2018

OK then, int_ it is.

@liadomide
Copy link
Member

Good. I will then apply this change soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants