-
Notifications
You must be signed in to change notification settings - Fork 7
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 2d/3d plotting support to base Dockerfile #7
Conversation
ping @rob-luke |
@bloyl could you please rebase so we can get the ci to run. |
There was discussion in #1 about having multiple images. A base or minimal image of smaller size. And a larger image that supports 3D. I think that until this repo reaches that level of sophistication (hopefully soon, it seems we have some momentum), that a larger image is ok. What do others think?
It would be good to get the ci to test this. @larsoner also had a test we should include. However, you mentioned elsewhere that you aren’t in to GitHub actions. If you wish I can open up a follow up pr for testing? I will run this locally tomorrow and give a proper review then |
It would be good to know how much bigger the 3D-support image is to decide if it's worth splitting |
@larsoner If I recall correctly the base image was about 800mb and this is
about 2gb. It would be relatively simple I think to have a
base worker and a gui worker. I can refactor to that in this pr if we want.
We would need to decide what non viz packages to include. For instance
Picard for ica or h5py for tfr io?
@rob-luke I'll rebase so that we can check building etc in this pr. Im
hesitant to add an action to run the example because i don't know how view
the generated output and with out that it's not much of a test.
…On Fri, Jun 25, 2021, 8:53 AM Eric Larson ***@***.***> wrote:
It would be good to know how much bigger the 3D-support image is to decide
if it's worth splitting
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKTXHJBAQCLG2YGSXGURXTTUR353ANCNFSM47HAMVPQ>
.rob-
|
No worries, you focus on the code and I can help with the tests etc. I cant edit your branch. Did you tick “allow maintainers to edit” when you opened the PR? Or maybe I am not an admin of this repo. I opened #8 to create a test for the visualisation. I have other tests in mind too. |
"Allow edits by maintainers" is checked I just added you as collaborator on my fork so hopefully that will give you access to do what you need to do. |
Awesome thanks. I will add the tests in. But first can you rebase or merge in main so that the newer CIs runs, and then I can edit the tests. Thanks |
@rob-luke you don't have to wait for this. At least a merge will be linear in history, so you can do:
and it will update his branch, and he can just do |
Rebase is done. |
This now makes 2 images mne-python (942MB) and mne-python-viz (2.18GB). I did also added a test that is really slow and does way too much. It may be overkill to be honest. It also fails because we can't write out which is what the current test tries to do. Happy to remove it until we have something better. |
@bloyl did you see my suggested test in #5 (comment) and then #8 ? To me something basic like this should be good enough to make sure that 3D works, and something similar could be done pretty easily with matplotlib to make sure 2D works. I would just start with this and add more complex tests if/when we see failures (e.g., someone finds that volume rendering doesn't work) |
I just pulled this test in favor of the one in #8. I do think that it would be useful have someway to do a full flex of what ends up being supported in mne by these environments. but that is another PR for sure. |
Looking pretty good now. Can you also update the readme to describe this information? Finally, can these images be used to start a Jupyter notebook/lab? Or just to run scripts? And I think we should wait for a review from @SherazKhan as we are modifying the base image he uses. |
There are only 2 images built:
I am not super I don't think that using
Neither image supports jupyter notebook or lab. I suspect bringing back the notebook image is the way to go for that. You could try to extend the Waiting for @SherazKhan is fine with me. |
Thanks for taking the time to walk me through it. We will need better documentation at some point. But I think we are in a state of flux, and can address the readme once this sequence of PRs is done.
Over at https://github.com/rob-luke/mne-nirs-docker I found that simply adding 'pip install jupyterlab' worked perfectly. But I can open a follow up PR for that (let's get this one merged ASAP, and easier to review small PRs). My final question (I know I've said that before) is can we change |
I would love to keep the momentum up with improvements to this repo. But any other changes rely on getting this base image right. Is there anything I can do to help get this reviewed @SherazKhan? |
Looks good to me @larsoner ? |
Thanks @SherazKhan, I will hit merge later today then. |
Turns out I dont have write access to this repo. @larsoner @agramfort could you please review and merge if happy. |
You have write access now 👏
|
Thanks @larsoner |
I hit merge rather than squash and merge. Sorry |
Add support for plotting along with some basics needed for a more complete mne toolset.
to test plotting via
mne.report
Build it
docker-compose build worker
Run example
docker run -it -v `pwd`/base/examples:/opt/app/examples mnetools/mne-python python /opt/app/examples/test_mne_report.py /opt/app/examples/mne_rpt.html
Examine generated report
firefox base/examples/mne_rpt.html
This does increase the size of image to
2.1 GB