Skip to content

Conversation

@nikhilchandra
Copy link
Contributor

@nikhilchandra nikhilchandra commented Apr 15, 2025

Fixes #3782

Nikhil Chandra and others added 2 commits April 15, 2025 17:25
@zm711
Copy link
Member

zm711 commented Apr 16, 2025

At least based on my reading of the errors it seems like they may have played with the arguments for this logging. Hopefully you don't end up needing to do too much logic. Also note we haven't updated for 4.0.31 yet so that test will fail even if you get the logging working on all other versions.

@nikhilchandra nikhilchandra marked this pull request as ready for review April 17, 2025 00:50
@nikhilchandra
Copy link
Contributor Author

@zm711 I think it works now up to v4.0.30, please confirm!

@alejoe91 alejoe91 added the sorters Related to sorters module label Apr 17, 2025
@zm711
Copy link
Member

zm711 commented Apr 17, 2025

yeah that seems to be working for all of them before 0.31. I think we review this now :) I'm a bit busy today and tomorrow, but I can look over this this weekend unless someone else has time before that.

Copy link
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

I think the only thing I'm not sure about is should we only do this logging if the user enters that they want spikeinterface in verbose mode? It seems like you removed the check for verbose @nikhilchandra but we try to require the user request verbose rather than have verbosity be the default. So I wonder if we should only have the logger if the user specifies verbose as a kwarg?

@nikhilchandra
Copy link
Contributor Author

nikhilchandra commented Apr 19, 2025

@zm711 In the original code, when verbose=True then the logging level for what gets printed to the console is INFO. Note that the default logging level is always WARNING.

In the new code, the newly inserted setup_logger() function from kilosort creates a StreamHandler that manages console output. In newer versions of Kilosort, setup_logger() has an input argument called verbose_console. When True, the StreamHandler's logging level is set to DEBUG. When False, it is set to INFO.

Thus, to replicate the original behavior of spike interface

  1. I always set the verbose_console argument to False-->this means that the StreamHandler's logging level is INFO
  2. If the spike interface user sets the verbose flag to True, then we can leave the StreamHandler's logging level as is
  3. If the spike interface user sets the verbose flag to False, then we can set the StreamHandler's logging level to WARNING

Note: older versions of Kilosort4 (4.0.16-4.0.18) don't name the logger "kilosort", instead opting to use a blank string "". In other versions, setup_logger() does not accept a verbose_console argument. The rest of my changes are just geared towards ensuring that the above logic works across all supported versions of Kilosort.

@zm711
Copy link
Member

zm711 commented Apr 19, 2025

Sorry I actually missed line 197 where you do the switch to warning if not verbose. This looks fine to me then. :)

Copy link
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

This is good by me now, @alejoe91 did you want to take a look?

@alejoe91
Copy link
Member

Mmm, I don't see the output when verbose=True and I also see progress bars when it is false:
image

In both cases though the log is correctly generated!

@nikhilchandra can you take a look?

@alejoe91 alejoe91 added this to the 0.102.3 milestone May 2, 2025
@alejoe91
Copy link
Member

alejoe91 commented May 6, 2025

Actually, I tested this again and it works. The problem is only that with verbose=False we still get progress bars...But I think this is a KS problem

@alejoe91
Copy link
Member

alejoe91 commented May 6, 2025

See MouseLand/Kilosort#935

@alejoe91 alejoe91 merged commit 87f9b91 into SpikeInterface:main May 6, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sorters Related to sorters module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SpikeInterface wrapper for Kilosort4 does not invoke default Kilosort4 logging function

3 participants