Skip to content

Conversation

@massich
Copy link
Contributor

@massich massich commented Jan 22, 2019

When working with #5839, I found some things that I'm not sure are ok.
Like the fact that meas_date and annotations.orig_time lose sync.

Do you think that if a user mess up with meas_date she/he should know what
she/he's doing? or shall we overwrite the set of the meas_date to manually
sync it? What do you think about the fact that raw.info['meas_date'] = 0 is
perfectly valid but raw.info['meas_date'] = 10 would break somewhere down the line 'cos meas_date is supposed to be a tupple?

Just some random thoughts...

@larsoner
Copy link
Member

Do you think that if a user mess up with meas_date she/he should know what she/he's doing?

Yes in general info entries should not be messed with. See the recent discussion of info['sfreq']. We should just add a .. warning:: about unintended consequences of / not supporting changes in the Info docstring.

@larsoner
Copy link
Member

What do you think about the fact that raw.info['meas_date'] = 0 is perfectly valid but raw.info['meas_date'] = 10 would break somewhere down the line 'cos meas_date is supposed to be a tupple?

I don't know why this would be the case, it seems like it should always check for tuple-ness. I thought we implemented a check like this sometime recently.

@massich
Copy link
Contributor Author

massich commented Jan 23, 2019

Plus I had some tmin error that I can no longer reproduce, and it's not in travis.

―――――――――――――――――――――――――――――――――――――――― test_5839_set_annotations_problem[meas_date1] ――――――――――――――――――――――――――――――――――――――――

meas_date = datetime.datetime(1970, 1, 1, 0, 0)

    @pytest.mark.parametrize('meas_date', [0, datetime.utcfromtimestamp(0)])
    def test_5839_set_annotations_problem(meas_date):
        """Test something weird I found."""
        # from datetime.datetime import utcfromtimestamp
        raw = RawArray(data=np.empty((10, 10)),
                       info=create_info(ch_names=10, sfreq=10., ),
                       first_samp=10)
        raw.info['meas_date'] = meas_date
        raw.set_annotations(annotations=Annotations(onset=[.5],
                                                    duration=[.2],
                                                    description='dummy',
>                                                   orig_time=None))

meas_date  = datetime.datetime(1970, 1, 1, 0, 0)
raw        = <[RuntimeWarning("Could not get size for self.info") raised in repr()] RawArray object at 0x7fdab6a767b8>

mne/io/tests/test_raw.py:298: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
mne/io/base.py:787: in set_annotations
    emit_warning=emit_warning)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <Annotations  |  1 segment : dummy (1), orig_time : 1969-12-31 23:00:01>, tmin = -3599.0, tmax = -3598.0
emit_warning = True

    def crop(self, tmin=None, tmax=None, emit_warning=False):
        """Remove all annotation that are outside of [tmin, tmax].
    
        The method operates inplace.
    
        Parameters
        ----------
        tmin : float | None
            Start time of selection in seconds.
        tmax : float | None
            End time of selection in seconds.
        emit_warning : bool
            Whether to emit warnings when limiting or omitting annotations.
            Defaults to False.
    
        Returns
        -------
        self : instance of Annotations
            The cropped Annotations object.
        """
        offset = 0 if self.orig_time is None else self.orig_time
        absolute_onset = self.onset + offset
        absolute_offset = absolute_onset + self.duration
    
        tmin = tmin if tmin is not None else absolute_onset.min()
        tmax = tmax if tmax is not None else absolute_offset.max()
    
        if tmin > tmax:
            raise ValueError('tmax should be greater than tmin.')
    
        if tmin < 0:
>           raise ValueError('tmin should be positive.')
E           ValueError: tmin should be positive.

absolute_offset = array([-3598.3])
absolute_onset = array([-3598.5])
emit_warning = True
offset     = -3599.0
self       = <Annotations  |  1 segment : dummy (1), orig_time : 1969-12-31 23:00:01>
tmax       = -3598.0
tmin       = -3599.0

mne/annotations.py:329: ValueError

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #5852 into master will decrease coverage by 1.76%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5852      +/-   ##
==========================================
- Coverage   89.23%   87.46%   -1.77%     
==========================================
  Files         417      417              
  Lines       75164    75185      +21     
  Branches    12376    12377       +1     
==========================================
- Hits        67071    65763    -1308     
- Misses       5216     6573    +1357     
+ Partials     2877     2849      -28

@massich massich added this to the 0.18 milestone May 15, 2019
@massich
Copy link
Contributor Author

massich commented May 15, 2019

If this is green, it can be closed. Its cover by #6313

@larsoner
Copy link
Member

It's not green. Still okay to close?

@massich
Copy link
Contributor Author

massich commented May 15, 2019

No, the operation order is broken. Do we care about though?

The problem is that if you set raw.annotations and then you change raw.info['meas_date'] the annotations are no longer valid and it's silent. We don't update the annotations nor raise no warning or error.
To do any of those things we need to capture the setting of the dictionary. I've no idea how to do that from the top of the head nor in a short time.

@larsoner if you (or anybody else) knows a quick fix for it feel free to take over and ping me. Otherwise, you can either label it as wont-fix or move it to 0.19 and I'll get back to it.

@larsoner
Copy link
Member

The problem is that if you set raw.annotations and then you change raw.info['meas_date'] the annotations are no longer valid and it's silent. We don't update the annotations nor raise no warning or error.

Users should not manually change entries in info, other than info['bads'], otherwise things will break. As long as things work under anonymization (which should be the only place we change info['meas_date'], I think) then it should be fine.

@massich massich closed this May 15, 2019
@drammock
Copy link
Member

drammock commented May 15, 2019 via email

@larsoner
Copy link
Member

I seem to recall this coming up before and ultimately not making it in. Maybe the discussion is somewhere here if anyone is interested in looking further: #2765

@massich
Copy link
Contributor Author

massich commented May 15, 2019

This means to modify the dictionary class. And I don't think it would be an easy fix. The anonymization works because we reset raw.annotations.orig_time.

I think that the most pragmatic thing is to assume that [in python we are all consenting adults](https://python-guide-chinese.readthedocs.io/zh_CN/latest/writing/style.html#we-are-all-consenting-adults
and if you mess up with info, it's at your own risk. I did a quick search in the issue tracker and I couldn't find anyone reporting a bug. But I guess that it would be good to state that in the documentation so that if someone messes it up, we can point them there.

@massich
Copy link
Contributor Author

massich commented May 15, 2019

The discussion about raw.info's contract is in #2860

@massich massich deleted the meas_date branch May 18, 2019 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants