-
Notifications
You must be signed in to change notification settings - Fork 216
More cleanup of imports focused mostly on testing and utils around testing #3841
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
Changes from all commits
3829f5a
f851a39
f1875c1
5bd9b82
6dacbc4
d9c1693
2762220
a04c9f4
c7b0d9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,8 +116,6 @@ def _set_params( | |
**other_kwargs, | ||
): | ||
|
||
import pandas as pd | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
|
||
# TODO alessio can you check this : this used to be in the function but now we have ComputeTemplateMetrics.function_factory() | ||
if include_multi_channel_metrics or ( | ||
metric_names is not None and any([m in get_multi_channel_template_metric_names() for m in metric_names]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm cool with committing this. Will do next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that comment is ... false, right?