More cleanup of imports focused mostly on testing and utils around testing #3841
Conversation
| @@ -24,6 +23,7 @@ | |||
| # ---------------------------------------------------------------------------------------------------------------------- | |||
|
|
|||
|
|
|||
| ### Shouldn't we skip if we can't find the package. In both these cases we skip if we find the packages.....##### | |||
There was a problem hiding this comment.
@alejoe91 actually I had one question about this PR while we work on fixing the testing suite? What is the point of this check? I understand that we skip if ON_GITHUB but why would we skip if we find the packages we need in order to run the test?
There was a problem hiding this comment.
Yeah logic is wrong! If you have the packages locally it should run!
|
@h-mayorquin any interest in taking a quick look at this :) |
| @@ -40,11 +40,10 @@ def has_plexon2_dependencies(): | |||
| return False | |||
|
|
|||
| # Check for 'zugbruecke' using pip | |||
There was a problem hiding this comment.
| # Check for 'zugbruecke' using pip |
There was a problem hiding this comment.
I'm cool with committing this. Will do next.
There was a problem hiding this comment.
yes, that comment is ... false, right?
| @@ -116,8 +116,6 @@ def _set_params( | |||
| **other_kwargs, | |||
| ): | |||
|
|
|||
| import pandas as pd | |||
There was a problem hiding this comment.
The import should throw an error at the init of anything that will need it downstream. Is this the case, here?
There was a problem hiding this comment.
If that's the case we can check check for the spec and raise an error. I can add that in a new commit.
There was a problem hiding this comment.
I am just wondering if this is a place where some things are initialized but I don't know template metrics.
There was a problem hiding this comment.
We are NOT currently using the package here. I think because we use it later Sam/Alessio just import it everywhere. Tests are passing so we are not directly using it at init. But you raise a good point that we should probably check that it is around at init for users!
|
thanks for your comments Heberto. Let me doublecheck on the pandas thing. |
I'm going to keep chipping away at this cleanup to try to add some margin speed gains in the library. :)
Also fixed a typo in a function name. This is pretty deep in the sorter utils. Maybe a deprecation, but I feel like no?