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

making caiman.run() common extension callbacks_finished arg optional #45

Merged
merged 23 commits into from
Jun 10, 2022

Conversation

clewis7
Copy link
Collaborator

@clewis7 clewis7 commented Jun 9, 2022

addressing issue #23

made callbacks_finished arg optional, should no longer cause problem of having to input arg for callbacks_finished as print[] when running function in jupyter lab!!

@kushalkolar
Copy link
Collaborator

@clewis7 thanks! You'll also have to change the code around here to something like if callbacks_finished is not none:

for f in callbacks_finished:

@@ -217,14 +217,16 @@ def get_temporal_components(
# TODO: Cache this globally so that a common upper cache limit is valid for ALL batch items
@validate("cnmf")
def get_reconstructed_movie(
self, ixs_frames: Tuple[int, int] = None, add_background: bool = True
self,
ixs_frames: Optional[Tuple[int, int], int] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

also tests are failing because this is wrong typing syntax, I think this is the correct syntax if you have multiple optional types:

Optional[Union[Tuple[int, int], int]]

@@ -487,6 +487,57 @@ def test_cnmf():
cnmf_max_actual = numpy.load(ground_truths_dir.joinpath("cnmf", "cnmf_std.npy"))
numpy.testing.assert_array_equal(cnmf_max, cnmf_max_actual)

# test to check passing optional ixs components to various functions
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's with all the tests stuff in this PR/branch?

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 think it's because you merged that other branch into dev perhaps, and then I pulled new changes into my dev and then rebased this branch to align with dev perhaps...I'm not sure why it did that honestly

@kushalkolar
Copy link
Collaborator

goodluck on the linalg test, don't worry about the code review until then!

@kushalkolar kushalkolar merged commit 72c6480 into dev Jun 10, 2022
@kushalkolar kushalkolar mentioned this pull request Jun 10, 2022
kushalkolar added a commit that referenced this pull request Jun 10, 2022
@kushalkolar kushalkolar added this to the v0.1 milestone Jul 3, 2022
@kushalkolar kushalkolar deleted the optional-args branch November 8, 2022 03:19
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