Refactor some tests

[massich]

There you go, we can discuss what does and does not make sense.

[DiGyt]

@massich

Thanks very much for that! this looks great.
The only thing i'd change is that the ascii files curry7_bdf_ascii_file are not used for the tests, (or did i miss them)?

Can we add them by simple doing:

@pytest.mark.parametrize('fname', [
    pytest.param(curry7_bdf_file, id='curry 7'),
    pytest.param(curry8_bdf_file, id='curry 8'),
    pytest.param(curry7_bdf_ascii_file, id='curry 7 ascii'),
    pytest.param(curry8_bdf_ascii_file, id='curry 8 ascii'),
])

I think it would be important to cover these too, since the data reading is done differently for ascii.

[codecov-io]

Codecov Report

Merging #1 into read_curry will decrease coverage by 0.01%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##           read_curry       #1      +/-   ##
==============================================
- Coverage       89.24%   89.23%   -0.02%     
==============================================
  Files             416      416              
  Lines           74715    74712       -3     
  Branches        12336    12335       -1     
==============================================
- Hits            66683    66669      -14     
- Misses           5166     5175       +9     
- Partials         2866     2868       +2

[DiGyt]

There are some values that turned out slightly different, maybe due to some floating point + rounding stuff? The data from these two files is read exactly the same, so i'd say this is the best we'd get of it...

[massich]

1e-3 it's quite a bit compared to 1e-6. It is not a problem to do those things, maybe the file is an edge case, the operations whatever.. All this is possible. If it's double checked and it's something expected:great, fine. But just setting it to the value that makes my test green might be a bad strategy. It might silent a problem that would be more comboluted to find further. At the very least a commed should be there stating that this behavior is unexpected, we don't understand why this happen and we should get back to it. In other words, we had to relax the constrain for reasons that we couldn't figure out. Apart from that, do you understood everything? Do you think you can apply similar ideas in other places?

…

On Thu, Jun 27, 2019, 07:33 Dirk Gütlin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mne/io/curry/tests/test_curry.py <#1 (comment)>: > + picks, start, stop = ["C3", "C4"], 200, 800 + assert_allclose( + raw.get_data(picks=picks, start=start, stop=stop), + bdf_curry_ref.get_data(picks=picks, start=start, stop=stop), + ) + + ***@***.***(do_warn) ***@***.***_testing_data ***@***.***('fname,tol', [ + pytest.param(curry7_rfDC_file, 1e-6, id='curry 7'), + pytest.param(curry8_rfDC_file, 1e-3, id='curry 8'), # XXX: why 1e-3? There are some values that turned out slightly different, maybe due to some floating point + rounding stuff? The data from these two files is read exactly the same, so i'd say this is the best we'd get of it... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1?email_source=notifications&email_token=ABVX5Y44WSE6TZKYCBGXYE3P4RGIZA5CNFSM4H3TAQWKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4ZS7UQ#pullrequestreview-255012818>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVX5Y6F4JL5EHTNVKRE6KLP4RGIZANCNFSM4H3TAQWA> .

[DiGyt]

@massich

At the very least a commed should be there stating that this behavior is unexpected, we don't understand why this happen and we should get back to it.

I'll add that in the next commit

Apart from that, do you understood everything? Do you think you can apply similar ideas in other places?

Yes, thank you very much for that. I already added the ASCII files to the new parametrized tests.
I think the coverage is already quite good.
The only thing i would add is a case where sfreq=0. I've just opened a PR for adding such a file to the test suite.