cisco / mindmeld

An Open Source Conversational AI Platform for Deep-Domain Voice Interfaces and Chatbots.
http://mindmeld.com
Apache License 2.0
677 stars 186 forks source link

Change behavior of param and param selection merging #427

Closed vrdn-23 closed 2 years ago

vrdn-23 commented 2 years ago

What is the change?

With this PR, we are introducing a new change in how Mindmeld deals with params and _paramselection in model configuration files.

What was the earlier behaviour?

What is the new behaviour?

Why are we doing this?

This change is introduced to for two main reasons: 1) In order to help facilitate the update process for scikit-learn, we need to eventually change the default solver for our text and tagger models. In order to make sure this change can be applied everywhere uniformly, it makes sense that we have it stored in our default configurations. This change also allows us to easily adapt to new configuration changes from the scikit-learn side of things with minimal code overhead for us. 2) The behavior of _paramselection when we also have params defined seems unintuitive. A user that defines a value in params is sure of it, and would want that value to be reflected across the model and expects a _paramselection across the range of values defined while also making sure these are the best params we can get with the fixed params values included. This new change implements this behavior.

ritvikshrivastava commented 2 years ago

Something I discussed with @vrdn-23 :

How about we get rid of the two configs and the need to merge them, and instead provide the developers with a single config that has tunable/fixed parameters and a boolean flag that determines whether grid search needs to be run; with each parameter only having one definition in the config rather than possibly having two as in the current implementation.

Something like:

params = {
   "penalty": "l2", 
   "C": 100, 
   "solver": "liblinear",
   "run_selection": False,
   ...
}

or

params = {
   "penalty": ["l1", "l2"],
   "C": [10,100,1000], 
   "solver": "liblinear",
   "run_selection": True,
   ...
}

This could be a design change t prevent developers from specifying the same parameters in both the static and variable parameter selection that could cause confusion; and also prevents the whole error/warning message feedback for the developers to fix this error.