Skip to content

MRG, ENH: Remove 15-char limit for FIF#8574

Merged
agramfort merged 2 commits intomne-tools:mainfrom
larsoner:rename-block
Feb 20, 2021
Merged

MRG, ENH: Remove 15-char limit for FIF#8574
agramfort merged 2 commits intomne-tools:mainfrom
larsoner:rename-block

Conversation

@larsoner
Copy link
Copy Markdown
Member

@larsoner larsoner commented Nov 25, 2020 •
edited
Loading

Closes #8348
Closes #8312

@larsoner larsoner mentioned this pull request Nov 25, 2020
Closed
6 tasks
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Dec 9, 2020

@agramfort this is ready for review from my end. Once we're happy with this, I can make the corresponding changes in MNE-MATLAB.

Should we try to get this into 0.22, or make it an early 0.23 PR? I'm leaning toward the latter so we at least get a few months in of dev use

@larsoner larsoner added this to the 0.23 milestone Dec 9, 2020
@agramfort
Copy link
Copy Markdown
Member

agramfort commented Dec 11, 2020

let's merge this after the release

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Dec 11, 2020

Agreed. @bloyl would you be interested in doing the companion PR to MNE-MATLAB? My MATLAB brain has atrophied over the last many years of using Python...

Base automatically changed from master to main January 23, 2021 18:27
@larsoner larsoner mentioned this pull request Feb 12, 2021
Merged
@larsoner larsoner changed the title ENH: Remove 15-char limit for FIF MRG, ENH: Remove 15-char limit for FIF Feb 12, 2021
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Feb 12, 2021

Companion PR is in mne-tools/mne-matlab#21, so these two are ready for review/merge from my end.

agramfort
agramfort reviewed Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
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.

works great !

just a few suggestions to make the code more self explanatory.

thx heaps @larsoner !

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Feb 19, 2021 via email

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Feb 19, 2021

it took me some time to understand that's why I suggested to rename to ch_names_mapping

I'm not sure what you're referring to here -- I made this change...?

Pushed a change to preserve the API of read_ctf_comp. Failures are just pip pre

@agramfort agramfort merged commit f9e62e0 into mne-tools:main Feb 20, 2021
@agramfort
Copy link
Copy Markdown
Member

agramfort commented Feb 20, 2021

thanks @larsoner !

@larsoner larsoner deleted the rename-block branch February 1, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.23

Development

Successfully merging this pull request may close these issues.

Why is the channel name length limited to 15 characters?

2 participants

@larsoner @agramfort