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

OnACID docs #1070

Merged
merged 8 commits into from
Jul 29, 2023
Merged

OnACID docs #1070

merged 8 commits into from
Jul 29, 2023

Conversation

kushalkolar
Copy link
Collaborator

@kushalkolar kushalkolar commented Apr 21, 2023

adding some docstrings and comments to OnACID

@kushalkolar kushalkolar marked this pull request as ready for review April 21, 2023 20:17
caiman/source_extraction/cnmf/online_cnmf.py Show resolved Hide resolved

Ain: csc_matrix, optional
binary masked for seeded initialization as a Compressed Sparse Column matrix.
To use set ``"init_method"`` to ``"seeded"``
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this is the case

caiman/source_extraction/cnmf/online_cnmf.py Show resolved Hide resolved
caiman/source_extraction/cnmf/online_cnmf.py Show resolved Hide resolved
Copy link
Member

@pgunn pgunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good


Args:
params: CNMFParams
CNMFParams object with parameters for the entire run of initialization and OnACID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for the entire run of" is weird phrasing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for the full pipeline: initialization -> motion correction -> source extraction"

Estimates object to load an existing model

path: str, optional
path to a saved OnACID model on disk
Copy link
Member

@pgunn pgunn May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we mention what format the model is in? Or point at a doc maybe for how to make one of these?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pat good point., We should probably have an example of this in use in a demo notebook at some poin, given that we want to move toward online as the next gen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assuming it's the hdf5 files? Anyways I think the better way to do this (and the estimates above) is to have a @classmethod. Such as OnACID.from_estimates(), OnACID.from_hdf(), etc.

path to a saved OnACID model on disk

dview:
dview instance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a bit more explanation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I describe it as a "multicore processing object" and think that might be a good generic description?

It would be nice if we had better general documentation on dview (in the setup_cluster() the docs for it are "dview: ipyparallel dview object, or for multiprocessing: Pool object". But if the users use SLURM I'm not sure what dview will return. I think "multicore processing object" or something similar will work for all use cases?.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea the name dview isn't great and I myself am not sure exactly what this is in different scenarios, feel free to edit/suggest

@kushalkolar
Copy link
Collaborator Author

let's merge this? OnACID is one of the least documented parts of the codebase and these will help.

@pgunn
Copy link
Member

pgunn commented Jul 29, 2023

Once it's marked no-longer-WIP I'm happy to give it final review

@kushalkolar
Copy link
Collaborator Author

Ah right, edited, ready for another look!

@kushalkolar
Copy link
Collaborator Author

edited! 😄

@pgunn pgunn merged commit bb588d3 into flatironinstitute:dev Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants