Skip to content

remove some ._filenames uses in favor of .filenames #12996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
larsoner merged 2 commits into from
Dec 2, 2024
Merged

remove some ._filenames uses in favor of .filenames #12996

larsoner merged 2 commits into from
Dec 2, 2024
+11 −11

Conversation

@sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Nov 28, 2024

following #12843

I think it'd be good to reduce the use of private attributes in our own code base as well, if we might as well use the public attribute.

sappelhoff
sappelhoff commented Nov 28, 2024
View reviewed changes
@larsoner
Copy link
Member

larsoner commented Nov 28, 2024

I don't have time to look deeply yet, just a quick comment that in the PR where we made the switch to paths and a public attribute, at some point I used git grep and tried to change the ones that I thought made sense to change, and left ones behind that didn't make sense to change or couldn't be changed. It's entirely possible I missed a few or I was wrong about what couldn't be changed though!

@sappelhoff
Copy link
Member Author

sappelhoff commented Nov 29, 2024

@larsoner @cbrnr the fix in c40c6f4 seems to have solved the failing tests. I believe the changes here can truly be converted to use the public attribute.

@cbrnr
Copy link
Contributor

cbrnr commented Nov 29, 2024

That's great and 👍 for merging, but I am still a bit concerned why only raw._filenames and not raw.filenames works, as these two are supposed to point to the same object, or no?

@larsoner larsoner merged commit ec77e7c into mne-tools:main Dec 2, 2024
@larsoner
Copy link
Member

larsoner commented Dec 2, 2024

Thanks @sappelhoff !

@sappelhoff sappelhoff deleted the filenames branch December 2, 2024 16:18
@sappelhoff
Copy link
Member Author

sappelhoff commented Dec 2, 2024

, but I am still a bit concerned why only raw._filenames and not raw.filenames works, as these two are supposed to point to the same object, or no?

See: #12996 (comment)

This is code in a small "helper class" that does not inherit anything from our other objects.

@cbrnr
Copy link
Contributor

cbrnr commented Dec 2, 2024

But why can it not use raw.filenames?

@sappelhoff
Copy link
Member Author

sappelhoff commented Dec 2, 2024

It probably can, if we make more code adjustments. Maybe worth a follow up in the future! :-)

larsoner added a commit to larsoner/mne-python that referenced this pull request Dec 6, 2024
@larsoner
Merge remote-tracking branch 'upstream/main' into cola5
0adefdf 
* upstream/main: (736 commits)
  ENH: Add round-trip channel name saving (mne-tools#13003)
  update governance (mne-tools#12896)
  pin selenium and stop filtering its warning (mne-tools#13000)
  DOC: fix return value doc of inst.get_montage() (mne-tools#12995)
  remove some ._filenames uses in favor of .filenames (mne-tools#12996)
  use eegbci.standardize instead of custom code (mne-tools#12997)
  Bump autofix-ci/action from dd55f44df8f7cdb7a6bf74c78677eb8acd40cd0a to ff86a557419858bb967097bfc916833f5647fa8c in the actions group (mne-tools#12999)
  MAINT: Update code credit (mne-tools#12998)
  MAINT: Unpin VTK (mne-tools#12991)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12988)
  Fix: update cnt.py to check missing annotation (mne-tools#12986)
  do not log about using set_montage AGAIN if it has never been used (mne-tools#12984)
  DOC: fix numpy docstr example Vectorizer() (mne-tools#12983)
  Changed epochs_clean_sub = model_plain.apply(epochs) to epochs_clean_sub = model_sub.apply(epochs), making use of the regression coefficients from model_sub. mne-tools#12977 (mne-tools#12978)
  DOC: add meegkit to related software suite (mne-tools#12976)
  DOC: specify tmax_raw param may be None, and what it means (mne-tools#12972)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12975)
  DOC: fix note in legacy pick_channels func (mne-tools#12973)
  Use `uint16_codec` argument (mne-tools#12971)
  Bump codecov/codecov-action from 4 to 5 in the actions group (mne-tools#12970)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drammock drammock Awaiting requested review from drammock drammock is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

@cbrnr cbrnr Awaiting requested review from cbrnr

@adam2392 adam2392 Awaiting requested review from adam2392 adam2392 is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@rob-luke rob-luke Awaiting requested review from rob-luke rob-luke is a code owner

Assignees

No one assigned

Labels

no-changelog-entry-needed skip-changelog-checks

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sappelhoff @larsoner @cbrnr