OMS-NetZero / FAIR

Finite-amplitude Impulse Response simple climate model
https://docs.fairmodel.net
Apache License 2.0
121 stars 61 forks source link

TCRECS function between Python 3.7 and 2.7 #66

Closed TeddyHornsby closed 4 years ago

TeddyHornsby commented 5 years ago

Describe the bug

Hi all, so I've been adapting some code and I think I've come across a translation issue between py2.7 and py3.6 when specifying the TCR & ECS ("TCRECS") values. the following code runs on py2.7.16 platforms but doesn't work on py3.7.3. i'm using jupyter notebook mainly but I've replicated the issue on several computers and platforms.

Failing Test

import fair
from fair.RCPs import rcp45
tcrecs = [1.5, 2.75]
C45, F45, T45 = fair.forward.fair_scm(emissions=rcp45.Emissions.emissions, tcrecs = tcrecs)

the error code on 3.7.3 platforms is:

IndexError: too many indices for array

The package tests all run fine on both but the stated examples for TCR ECS experiments also fail on 3.7.3.

Potential Work Around - but issues the following solves this issue:

emissions = rcp45.Emissions.emissions
tcr = 1.5
ecs = 2.75
tcr_model = (np.zeros(len(emissions)) + tcr)
ecs_model = (np.zeros(len(emissions)) + ecs)
tcrecs = np.vstack([tcr_model, ecs_model]).T
C45, F45, T45 = fair.forward.fair_scm(emissions=rcp45.Emissions.emissions, tcrecs = tcrecs)

however as TCRECS here has to be specified as the same length as the emissions array I am concerned that this might impact ensemble runs in 3.7.3 ie. TCRECS is being specified for each year rather than each run?

Anyway I hope this helps! Many thanks.

chrisroadmap commented 5 years ago

Hi @TeddyHornsby, thanks for this. I can also replicate this issue in py3.6, whereas in py2.7 it doesn't raise an error. The workaround you suggest forces tcrecs to be an array and once tcrecs is an array everything works smoothly. I'll work on a bug fix for this. Cheers again.

chrisroadmap commented 5 years ago

The error is in this section of forward.py

    # Convert any list to a numpy array for (a) speed and (b) consistency.
    # Goes through all variables in scope and converts them.
    frame = inspect.currentframe()
    args, _, _, values = inspect.getargvalues(frame)
    for arg_to_check in args:
        if type(values[arg_to_check]) is list:
            exec(arg_to_check + '= np.array(' + arg_to_check + ')')

which in python 2 would convert any variable in scope that was a list into a numpy array. What is very strange is that I've added a new test that includes your example above, and it passes in python 3. I'll need to think some more about what to do here, but for now the workaround is to pass every argument to fair_scm that could possibly be a list as an array.

chrisroadmap commented 5 years ago

This test, which should fail based on the behaviour above, passes: https://github.com/OMS-NetZero/FAIR/compare/fix-py3-tcrecs

So it seems that if running interactively, you need to pass arguments to fair_scm as a numpy array explicitly. This is the python 3 workaround for now. For v2.0 I'll work on a smarter solution.

TeddyHornsby commented 5 years ago

Thanks Chris, will use the workaround for now, appreciate!

chrisroadmap commented 4 years ago

We don't claim to support python 2 any more, so I'm going to close this issue. Solution is to pass anything that could be a list as a numpy array.