-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add mne-connectivity #17982
Add mne-connectivity #17982
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/mne-connectivity:
For recipes/mne-connectivity:
Documentation on acceptable licenses can be found here. |
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/mne-connectivity:
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@larsoner I've added you as a co-maintainer; please confirm if that's okay with you, thanks! |
extra: | ||
recipe-maintainers: | ||
- hoechenberger | ||
- larsoner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm happy to be a maintainer!
@conda-forge-admin, please restart ci |
@conda-forge/staged-recipes, this one is ready for review. Thank you! |
recipes/mne-connectivity/meta.yaml
Outdated
- python >=3.7 | ||
- mne-base >=0.24 | ||
- numpy | ||
- scipy | ||
- xarray | ||
- netCDF4 | ||
- pandas | ||
- scikit-learn | ||
- pyvista >=0.30 | ||
- pyvistaqt >=0.4 | ||
- sip | ||
- pyqt | ||
- pyqt5-sip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These requirements don't align with what upstream has. Are you sure they are correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification of the upstream requirements is a bit sloppy in setup.py
; the "actual" deps are in requirements.txt
:
numpy
scipy
mne>=0.24
xarray
netCDF4
h5netcdf
pooch
tqdm
matplotlib
pyqt5>=5.10,<5.14; platform_system == "Darwin"
pyqt5>=5.10,!=5.15.2,!=5.15.3; platform_system == "Linux"
pyqt5>=5.10,!=5.15.3; platform_system != "Linux" and platform_system != "Darwin"
pyqt5-sip
sip
pyvista>=0.30
pyvistaqt>=0.4
pandas
cc @larsoner could you confirm this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I see I missed a few, I'll update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks right. setup.py is intentionally minimal, we should probably add a [full] option someday...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BastianZim I think with 829b34a now the dependencies of this package are in sync with upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find scikit-learn
upstream otherwise yes, looks the same now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, but scikit-learn
is listed in their installation instructions:
https://mne.tools/mne-connectivity/dev/install.html
It seems everything is slightly out of sync here :\ (setup.py, requirements.txt, installation instructions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh ok thanks. There, minimum versions are listed, those should probably be included here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea if those listed are up to date. But since they usually don't decrease as time passes, I think I'll just list them here, alright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, done! Now we have the best melange of all three: install docs, requirements file, setup.py.
My job here is done! 👩🍳
😂
Thank you, @BastianZim! |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).