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

Sampling interval can be float #59

Merged
merged 6 commits into from Nov 7, 2020
Merged

Conversation

cbrnr
Copy link
Collaborator

@cbrnr cbrnr commented Nov 7, 2020

Fixes #52. I've also introduced f-strings in this block of code - do you want me to convert the rest as well?

@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #59 (19a1c40) into master (f1aecaa) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   97.19%   97.26%   +0.07%     
==========================================
  Files           3        3              
  Lines         285      293       +8     
==========================================
+ Hits          277      285       +8     
  Misses          8        8              
Impacted Files Coverage Δ
pybv/io.py 99.37% <100.00%> (ø)
pybv/tests/test_bv_writer.py 94.61% <100.00%> (+0.35%) ⬆️

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 f1aecaa...19a1c40. Read the comment docs.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Cool, thanks.

Can you please add a test covering a bunch of conventionally used sfreqs? (e.g. 512, 125, 100, 500, 1000, 1024, ...)

re: f-strings yes, would be nice! Preferably as a follow up PR though!

@sappelhoff
Copy link
Member

Finally, please add a what's new entry --> I think this would fall under the "bug" category, WDYT?

@sappelhoff sappelhoff added this to the 0.4.0 milestone Nov 7, 2020
@sappelhoff sappelhoff added the fix fixes a bug label Nov 7, 2020
@cbrnr
Copy link
Collaborator Author

cbrnr commented Nov 7, 2020

@sappelhoff is this test OK?

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
docs/changelog.rst Outdated Show resolved Hide resolved
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

LGTM apart from my two suggestions.

If tests pass, this can be merged!

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@cbrnr
Copy link
Collaborator Author

cbrnr commented Nov 7, 2020

But do you think it's necessary to read the interval from the VHDR file and compare that with the inverse of the specified sampling frequency? Or do we just rely on MNE to do that during loading?

@sappelhoff
Copy link
Member

But do you think it's necessary to read the interval from the VHDR file and compare that with the inverse of the specified sampling frequency? Or do we just rely on MNE to do that during loading?

I think ideally, we'd do both 🤔 but for now MNE is probably sufficient.

@sappelhoff sappelhoff merged commit 4e71426 into bids-standard:master Nov 7, 2020
@sappelhoff
Copy link
Member

Thanks @cbrnr if you are up to add a test with bare-python reading+checking the written sfreq, that'd be a nice addon :)

@cbrnr cbrnr deleted the sfreq-float branch November 7, 2020 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

float sampling frequencies are not supported
2 participants