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

initial draft of new TD-fNIRS KF2 example #586

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

JohnGriffiths
Copy link

@JohnGriffiths JohnGriffiths commented Nov 13, 2024

Following from this PR discussion, this example demonstrates I/O of TD-FNIRS data, and also serves as an introduction to KF2 data.

Code and data based on example code from @julien-dubois-k

Remaining to-do:

  • Modify installation target to the above-linked PR
  • Add data download functionality
  • Modify some of the hdf access bits with references to getting the info from the mne snirf loader

@JohnGriffiths JohnGriffiths marked this pull request as ready for review November 14, 2024 06:05
@JohnGriffiths
Copy link
Author

PR adding KF2 tutorial example, as discussed.

I have verified that this builds an correct-looking example webpage with Make html

@larsoner - do need some help here with getting the CI to pass

Some things to note re: the contents of this example:

  1. It currently uses gdown to pull the example file. I suggest we keep this as-in, and consider updating to something with mne-misc-data after this one is merged

  2. Likewise, I suggest we edit the mne dependency back to the mne-tools/mne-python master in a later PR, once the PR in the above-linked thread has been resolved.

  3. Some of the contents regarding pulling stuff from the hdf5 archive could be modified in future versions to show pulling this info from the hdf archive directly or from the mne reader. However I don't believe this functionality is in the MNE .snirf reader yet, so we would need to complete a new PR with that code first.

@larsoner
Copy link
Member

FYI I am a bit busy for the next two weeks then should be able to come back to this!

@larsoner
Copy link
Member

Don't worry about the pip-pre failure it should be fixed by nilearn/nilearn#4724 soon. Can just work on getting the example to run and look good!

@larsoner
Copy link
Member

generating gallery for auto_examples/general... [ 27%] plot_11b_kf2_fingertapping.py
make: *** [Makefile:60: html] Killed

Probably due to memory usage

Screenshot 2024-11-18 at 11 39 57 AM

Is there some easy / reasonable way to reduce the memory usage? It's good if examples don't use a ton of memory (e.g., try to keep to < 2 GB).

In a worst case we could use a larger docker image but it would be nice to avoid it if possible

@JohnGriffiths
Copy link
Author

Is there some easy / reasonable way to reduce the memory usage? It's good if examples don't use a ton of memory (e.g., try to keep to < 2 GB)

If it's an option to keep as-is with larger docker image can we go with that for now?

It would be helpful to focus on the code needed to get this up and running first, and circle back to optimizations next.

I think RAM usage limits are also a more general question that would inform a lot of what we decide to add examples for on this topic (e.g. connectivity, source analysis, etc.).

@larsoner
Copy link
Member

It would be helpful to focus on the code needed to get this up and running first, and circle back to optimizations next.

Okay I pushed a commit that should do it.

FWIW bumping the resource class actually costs the mne-tools org compute credits because it's not part of the free tier so we should try to avoid keeping it that way as well long-term if possible unless there is a really compelling use case that we can't get memory usage down for. Even in MNE-Python so far we have managed to avoid this, only in MNE-BIDS-Pipeline have we needed to use it. So I'm cautiously optimistic we might be able to keep memory usage low here, too.

@JohnGriffiths
Copy link
Author

It would be helpful to focus on the code needed to get this up and running first, and circle back to optimizations next.

Okay I pushed a commit that should do it.

FWIW bumping the resource class actually costs the mne-tools org compute credits because it's not part of the free tier so we should try to avoid keeping it that way as well long-term if possible unless there is a really compelling use case that we can't get memory usage down for. Even in MNE-Python so far we have managed to avoid this, only in MNE-BIDS-Pipeline have we needed to use it. So I'm cautiously optimistic we might be able to keep memory usage low here, too.

Good, thanks.

Agree to avoiding this being a long-term solution.

@larsoner
Copy link
Member

@JohnGriffiths does the rendered example look right so far?

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.

2 participants