Skip to content
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.

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

[MRG] Fix copyfile_eeglab type conflicts #1126

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Mar 8, 2023

PR Description

Fix #1122

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

@welcome
Copy link

welcome bot commented Mar 8, 2023

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

@laemtl laemtl force-pushed the copyfile_eeglab_type_conflict branch from 78c014f to 5e6efd4 Compare March 8, 2023 18:25
doc/whats_new.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #1126 (074ea18) into main (0b1a61d) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1126      +/-   ##
==========================================
- Coverage   95.29%   95.23%   -0.06%     
==========================================
  Files          40       40              
  Lines        8670     8671       +1     
==========================================
- Hits         8262     8258       -4     
- Misses        408      413       +5     
Impacted Files Coverage Δ
mne_bids/copyfiles.py 97.72% <100.00%> (+0.01%) ⬆️
mne_bids/tests/conftest.py 93.75% <0.00%> (-6.25%) ⬇️
mne_bids/tests/test_write.py 90.17% <0.00%> (-0.15%) ⬇️
mne_bids/write.py 97.18% <0.00%> (-0.11%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mne_bids/copyfiles.py Outdated Show resolved Hide resolved
@sappelhoff
Copy link
Member

circleci was a bit weird:

image

never saw that before ... anyone else? Manually restarting the workflow seems to have helped

@adam2392
Copy link
Member

adam2392 commented Mar 8, 2023

circleci was a bit weird:

image

never saw that before ... anyone else? Manually restarting the workflow seems to have helped

We saw it in mne-connectivity too: mne-tools/mne-connectivity#129 (comment)

@laemtl laemtl force-pushed the copyfile_eeglab_type_conflict branch from 3df063b to ce70f52 Compare March 8, 2023 20:01
@laemtl laemtl requested a review from hoechenberger March 8, 2023 20:02
oldstyle = False
if 'EEG' in eeg:
eeg = eeg['EEG']
oldstyle = True

has_fdt_link = False
Copy link
Member

@hoechenberger hoechenberger Mar 8, 2023

Choose a reason for hiding this comment

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

I would suggest to revert this change, as it makes the program flow less obvious

Copy link
Contributor Author

@laemtl laemtl Mar 8, 2023

Choose a reason for hiding this comment

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

In the current implementation, there is some potential cases where has_fdt_link will not be set and an error thrown. It happened to me during testing so that's how I tried to fix that. Would you prefer an else case instead?

    try:
        # If the data field is a string, it points to a .fdt file in src dir
        if isinstance(eeg['data'][0, 0][0], str):
            has_fdt_link = True
        else:
            has_fdt_link = False
    except IndexError:
        has_fdt_link = False

Copy link
Member

Choose a reason for hiding this comment

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

For me both ways are fine. Thanks for figuring out that there is one potential flow that previously lead to an error!

@sappelhoff
Copy link
Member

you need to add yourself here as well @laemtl

.. _Moritz Gerster: http://moritz-gerster.com

@sappelhoff
Copy link
Member

and here, just behind Moritz, please:

- given-names: Moritz

@sappelhoff sappelhoff merged commit 841e14d into mne-tools:main Mar 8, 2023
@welcome
Copy link

welcome bot commented Mar 8, 2023

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

@sappelhoff
Copy link
Member

Thanks @laemtl 🚀

@sappelhoff sappelhoff changed the title Fix copyfile_eeglab type conflicts [MRG] Fix copyfile_eeglab type conflicts Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

write_raw_bids does not produce a valid SET file
5 participants