Clean paradigm #316
Clean paradigm #316
Conversation
| @@ -48,13 +48,7 @@ def __init__(self, con_id=None, onset=None, amplitude=None): | |||
| self.amplitude = amplitude | |||
| if con_id is not None: | |||
| self.n_events = len(con_id) | |||
| try: | |||
There was a problem hiding this comment.
No need for this now? Need deprecation warning or informative error?
There was a problem hiding this comment.
I'm still worried by backwards compatibility - I think previous behavior was:
import nipy.modalities.fmri.experimental_paradigm as ep
In [2]: ep.Paradigm([1, 2, 3]).con_id
Out[2]:
array(['c1', 'c2', 'c3'],
dtype='|S2')
where behavior on this branch is:
In [3]: ep.Paradigm([1, 2, 3]).con_id
Out[3]:
array(['1', '2', '3'],
dtype='|S1')
I can see that it's a good idea not to convert strings to ints then back to strings, but maybe preserve c prefix if condition names are integers?
There was a problem hiding this comment.
I think that in general it is a bad idea to decide that the conditions cannot be named '1', '2' etc. and to change this for the user. Now, if you think that backward compatibility is more important, I can revert that one.
There was a problem hiding this comment.
It's OK, I can see arguments both ways...
|
Bertrand - do you have time to look at these small things? |
|
This should be OK now. |
|
Sorry - so - where are we with issue 310? |
|
For the moment, we raise a warning to avoid breaking compatibility. The behavior will be changed in the next release. |
| self.con_id = np.array(['c' + str(int(float(c))) | ||
| for c in con_id]) | ||
| for c in con_id]) | ||
| except: |
There was a problem hiding this comment.
What error are you trying to catch with the except? Should try and avoid bare except if at all possible.
bthirion
force-pushed
the
clean_paradigm
branch
from
125e352
to
8f21759
Compare
bthirion
force-pushed
the
clean_paradigm
branch
from
8f21759
to
ca0d09f
Compare
MRG: fix condition names for paradigm Addresses issue #310
|
Thanks - sorry to be slow in merging. |
Addresses issue #310