Skip to content

Conversation

@jsosulski
Copy link
Contributor

Reference issue

Fixes #9374

What does this implement/fix?

Set include_tmax=True when tmax is larger than the epoch.

Additional Information

I think this cannot be sensibly done in the _time_mask function as we do not know whether tmax was set automatically or not in that function. However, I do now knot if _time_mask should do this kind of error handling or if it is up to the calling functions.

@welcome
Copy link

welcome bot commented May 6, 2021

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

@agramfort
Copy link
Member

The test failure seems unrelated.

@jsosulski can you add a line in the bug fix section of our what’s new page? See file latest.inc

Thx !

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

thx @jsosulski that's perfect !

@agramfort
Copy link
Member

@larsoner I let you merge when you have a minute !

@jsosulski
Copy link
Contributor Author

@agramfort : I just finished checking all the _time_mask calls in MNE and Evoked has the same issue. Should I append that fix to this PR or create a new Issue for Evoked first?

@agramfort
Copy link
Member

agramfort commented May 7, 2021 via email

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

thx @jsosulski

@larsoner ok with the fix?

@larsoner larsoner merged commit 908f52a into mne-tools:main May 10, 2021
@welcome
Copy link

welcome bot commented May 10, 2021

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@larsoner
Copy link
Member

Thanks @jsosulski !

larsoner added a commit to agramfort/mne-python that referenced this pull request May 10, 2021
* upstream/main:
  FIX: make epoch cropping idempotent (mne-tools#9378)
  MRG, ENH: Add NIRSport support (mne-tools#9348)
  MRG, ENH: Make _get_hpi_info public (mne-tools#9369)
  ENH: Add a playback button to the notebook 3d backend (mne-tools#8741)
  better docs for permutation_cluster_test (mne-tools#9365)
  MRG: Add fNIRS to html output (mne-tools#9367)
  When plotting GFP comparison in Report, don't show sensor layout by default (mne-tools#9366)
  DOC: Update Mayavi troubleshooting section (mne-tools#9362)
  more tutorial tweaks (mne-tools#9359)
  MRG, MAINT: Use native GitHub Actions skip (mne-tools#9361)
  MAINT: Clean up crufty code [circle front] (mne-tools#9358)
  API: Complete deprecations (mne-tools#9356)
  Add qdarkstyle, darkdetect to environment.yml [circle full] (mne-tools#9357)
  FIX: Fix
  FIX: Add
@jsosulski jsosulski deleted the make_epoch_crop_idempotent branch May 10, 2021 12:58
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.

Repeated Epoch/Evoked cropping is not idempotent when include_tmax=False

3 participants