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] Remove _estimate_line_freq #506

Merged
merged 8 commits into from Aug 21, 2020

Conversation

alexrockhill
Copy link
Contributor

@alexrockhill alexrockhill commented Aug 19, 2020

Followup to #503.

Describe your PR here

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"
  • Commit history does not contain any merge commits

updated whats_new

fixed line freq test

better formatting per Alex

Mainak request

good catch Alex

small word change for email

more descriptive name

reduced to at least one channel for line_freq

try n/a

try more n/a

tried improving powerline estimator

remove line freq estimation
@agramfort
Copy link
Member

@mne-tools/mne-bids-contributors apparently validator requires PowerLineFrequency to be specified. It cannot be 'n/a'

any opinion on this ?

2 options:

  • make it optional in standard/validator
  • prevent people from writing bids files without specifying (at worse manually) the PowerLineFrequency

@jasmainak
Copy link
Member

Option 2 makes more sense. To check while writing.

@agramfort
Copy link
Member

ok I'll go with 2 here... doing it now

@agramfort agramfort changed the title [WIP, BUG] Remove _estimate_line_freq [MRG] Remove _estimate_line_freq Aug 21, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #506 into master will decrease coverage by 0.05%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
- Coverage   92.79%   92.74%   -0.06%     
==========================================
  Files          14       14              
  Lines        2055     2026      -29     
==========================================
- Hits         1907     1879      -28     
+ Misses        148      147       -1     
Impacted Files Coverage Δ
mne_bids/utils.py 94.58% <ø> (-0.16%) ⬇️
mne_bids/read.py 95.52% <80.00%> (-0.79%) ⬇️
mne_bids/commands/mne_bids_raw_to_bids.py 94.11% <100.00%> (+0.56%) ⬆️
mne_bids/write.py 96.74% <100.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a8852a...76164dd. Read the comment docs.

@hoechenberger hoechenberger merged commit a6891bf into mne-tools:master Aug 21, 2020
@hoechenberger
Copy link
Member

Thanks, @alexrockhill and @agramfort!

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.

None yet

5 participants