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

Use assert_allclose instead of np.allclose and np.isclose(...).all() #115

Merged
merged 16 commits into from
Aug 17, 2023

Conversation

mscheltienne
Copy link
Collaborator

Fixes #114

Also change to assert_allclose which has a saner atol=0 argument + better logging/error information when failing.
Waiting now for tests to run, check that everything is still green.

After, WDYT about changing black to the default 88 character per line to match MNE? (it's a bit annoying to change the ruler position in the IDE 😉)

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #115 (40c1225) into main (38c8d48) will decrease coverage by 0.08%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
- Coverage   90.94%   90.86%   -0.08%     
==========================================
  Files          30       30              
  Lines        2616     2617       +1     
  Branches      501      501              
==========================================
- Hits         2379     2378       -1     
- Misses        136      137       +1     
- Partials      101      102       +1     
Files Changed Coverage Δ
pycrostates/cluster/utils/utils.py 85.71% <ø> (ø)
pycrostates/datasets/lemon/lemon.py 92.85% <ø> (ø)
pycrostates/metrics/davies_bouldin.py 95.00% <ø> (ø)
pycrostates/metrics/silhouette.py 100.00% <ø> (ø)
pycrostates/segmentation/segmentation.py 95.00% <ø> (ø)
pycrostates/utils/_docs.py 92.00% <0.00%> (ø)
pycrostates/viz/segmentation.py 77.96% <ø> (ø)
pycrostates/segmentation/_base.py 90.29% <70.00%> (ø)
pycrostates/io/fiff.py 77.81% <80.00%> (ø)
pycrostates/cluster/_base.py 90.47% <90.90%> (ø)
... and 16 more

@mscheltienne
Copy link
Collaborator Author

LMK if the changes are OK for you, else the commits can be reverted.

@mscheltienne mscheltienne requested a review from vferat July 3, 2023 09:14
@mscheltienne
Copy link
Collaborator Author

doc-build issue is likely due to joblib/joblib#1457
should be checked/fixed in MNE directly.

@mscheltienne
Copy link
Collaborator Author

mscheltienne commented Jul 25, 2023

The documentation build issue will be fixed when mne-tools/mne-python#11756 lands on main. Or we can build the documentation using mne-dev, but I don't think it's worth the trouble.

EDIT: MNE 1.5 is almost out, so it will be fixed soon.

@mscheltienne
Copy link
Collaborator Author

mscheltienne commented Aug 16, 2023

@vferat This PR is good to go. Don't bother reviewing the changes (since they are mostly due to the black autorun), but instead the bullet points bellow and let me know if you want me to revert/change anything:

  • Use saner assert_allclose in tests, from numpy.testing
  • Fix a couple of missing f-strings, spaces, style, ..
  • Remove flake8 in favor of Ruff (faster)
  • Change line-length to 88 characters (black default, MNE new default)
  • Fix documentation build with MNE 1.5. Note that depending on the version of MNE installed, some methods might not be accessible. The ChInfo class gained methods such as rename_channels, set_channel_types, plot_sensors if MNE 1.5 is installed because of re-arrangements in the MNE mixins.

If that's all good for you, I'll merge and then update #112 and fix the conflicts before reviewing.

@vferat
Copy link
Owner

vferat commented Aug 17, 2023

That's all good for me, I'll let you do the merge 👍

@mscheltienne mscheltienne merged commit 1d839e2 into vferat:main Aug 17, 2023
28 of 31 checks passed
@mscheltienne mscheltienne deleted the maint-fixes branch August 17, 2023 18:01
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.

Wrong Warning in ModK.plot()
2 participants