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

Fix compatibility issues with Python 2 #1004

Merged
merged 4 commits into from Aug 17, 2018
Merged

Conversation

@hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Aug 14, 2018

This PR contains some fixes for compatibility issues when using Python 2 when running examples and model_details scripts.

Python 2 doesn't handle complex numbers as elegantly as Python 3, which led to an error when running HillTononiModels.ipynb using Python 2. The problematic variable is now explicitly converted to a complex number if needed. There were also some minor issues in the PyNEST examples.

@hakonsbm hakonsbm changed the title Fix issue with complex numbers when running HillTononiModels.ipynb with Python 2 Fix compatibility issues with Python 2 Aug 14, 2018
@heplesser heplesser requested a review from stinebuu Aug 14, 2018
@@ -64,6 +64,7 @@
'''

from __future__ import print_function

This comment has been minimized.

@heplesser

heplesser Aug 14, 2018
Contributor

Why do we need this future import?

This comment has been minimized.

@hakonsbm

hakonsbm Aug 14, 2018
Author Contributor

Because of the end=' ' here:

print('Statistics after optimization:', end=' ')

This comment has been minimized.

@heplesser

heplesser Aug 14, 2018
Contributor

OK.

@@ -1261,6 +1261,11 @@
" self.hp = ht_params\n",
" \n",
" def m_inf(self, D):\n",
" # Workaround for handling complex numbers in Python 2\n",

This comment has been minimized.

@heplesser

heplesser Aug 14, 2018
Contributor

Hm, I am not really happy with this work-around, it is a lot of distracting code for a notebook. And can D really become negative? From the math, we should always have D>0 strictly.

This comment has been minimized.

@hakonsbm

hakonsbm Aug 14, 2018
Author Contributor

I agree that it's not an elegant work-around. m_inf is called from voltage_clamp like this

m_old = channel.m_inf(DT_V_seq[0][1])

where channel is iDK and DT_V_seq is the list in

nr, cr = voltage_clamp(iDK, [(500, -65.), (500, -35.), (500, -25.), (500, 0.), (5000, -70.)],
                      nest_dt=1.)

This comment has been minimized.

@heplesser

heplesser Aug 14, 2018
Contributor

So I guess the problem is that in

    def compute_I(self, t, V, m0, h0, D0):
        self.D = si.odeint(self.dD, D0, t, args=(V,))
        self.m = self.m_inf(self.D)
        return - self.hp['g_peak_KNa'] * self.m * (V - self.hp['E_rev_KNa'])

self.D may contain negative values (numerical error from odeint). But we know that D must always be strictly positive, so we should (i) check that we only get numerical noise below zero and then (ii) replace negative values in the result of odeint() with the smallest possible positive number.

This comment has been minimized.

@hakonsbm

hakonsbm Aug 15, 2018
Author Contributor

I think you misunderstood my previous comment. voltage_clamp() is called with a channel, and a list of interval-voltage pairs referred to as DT_V_seq. In voltage_clamp we have

# Control part
t_old = 0.
try:
    m_old = channel.m_inf(DT_V_seq[0][1])
except NotImplementedError:
    m_old = None

where DT_V_seq[0][1] is the voltage of the first interval-voltage pair, i.e. -65.0.

This is before it calls compute_I(). And I checked the values for self.D, they are all positive.

heplesser and others added 2 commits Aug 16, 2018
Corrected implementation in pure-python version of I_DK
Copy link
Contributor

@stinebuu stinebuu left a comment

I approve!

@heplesser heplesser merged commit ef46f5e into nest:master Aug 17, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hakonsbm hakonsbm deleted the hakonsbm:fix_complex_python2 branch Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.