Verbose config check #483
Verbose config check #483
Conversation
src/triage/experiments/validate.py
Outdated
| @@ -328,16 +328,19 @@ def _validate_imputations(self, aggregation_config): | |||
| agg_types = ["aggregates", "categoricals", "array_categoricals"] | |||
|
|
|||
| for agg_type in agg_types: | |||
| logging.info('agg_type:{}'.format(agg_type)) | |||
There was a problem hiding this comment.
Use logging variables instead of formatted strings:
logging.info('agg_type: %s', agg_type)
This has two advantanges:
- logging module doesn't do the interpolation unless the message is actually visible (minor win)
- and enables log aggregators to more intelligently aggregate logfiles. For instance, if a message gets logged multiple times with different arguments, log aggregators are smart enough to aggregate them together but this is not possible with formatted strings.
And the intros should be a bit more descriptive. 'Checking imputation rules for aggregation type %s', 'Checking imputation rules for aggregation %s', 'Checking imputation rules for metric %s'
| @@ -110,4 +112,5 @@ def visualize_chops(chopper, show_as_of_times=True, show_boundaries=True): | |||
| ax[0].set_title("Timechop: Temporal cross-validation blocks") | |||
| fig.subplots_adjust(hspace=0) | |||
| plt.setp([a.get_xticklabels() for a in fig.axes[:-1]], visible=False) | |||
| plt.savefig('timechop.png') | |||
There was a problem hiding this comment.
Isn't this going to break (and never call show) if whatever is running doesn't have write access to whatever directory this ends up being? Furthermore, if this breaks in a notebook setting (when the user is just trying to plot it to the screen), it will break their simple displaying use case for the sake of filesystem saving functionality that they don't even care about.
I think there's a lot that we can do to make this more robust (integrate it with the ProjectStorage via the CLI, etc) but the bare minimum for this pull request I think should be to just plot to an optional target instead of hardcoding a filesystem path. We do this in the audition plotting module for an example.
Just have the save target be an optional argument, and conditionally save it like this:
if path_to_save:
plt.savefig(path_to_save)
And we can make the implementation more complete in a future PR
This PR adds: