Question/Discussion of MLModel class

[janash]

I was just reviewing the code in this package and I started with the MLModel class. Currently, this is in a module called mlModels. (https://github.com/JasonGibsonUfl/quantumMLdev/blob/4b02be35d2bc1823dcf823965b4a81299bf752dd/quantumml/mlModels.py).

Here are a few suggestions regarding small changes that can or should be made:

1. PEP 8 recommends that module names be all lowercase (https://www.python.org/dev/peps/pep-0008/#package-and-module-names). The module name could be changed from mlModels to mlmodels or even ml_models
2. On line 194 the variable system is being concatenated with some strings. If someone passes in a variable which is not a string, you will likely get a type error. Check for type of input variable or catch the error to give a more descriptive error message.
3. get_ml_model doesn't seem to return anything at the moment.

On a large-scale note, I think we should really think about the structure of this module. Considering that this module contains one class and this class has only has static methods and a constructor (__init__), I really don't see the need for a class here. The class does store some data, but the functions do not act on that data at all. I would recommend moving them out of the class. All putting them in a class currently does is namespace the functions MLModel.function_name which I'm not sure is even necessary. The same thing could be accomplished by putting the functions in a module.

I understand that the MLModel is supposed to be similar to models from scikitlearn. If you examine something like the LinearRegression model in scikitlearn, you'll notice that it has a constructor and an associated method (fit for LinearRegression) See here - https://github.com/scikit-learn/scikit-learn/blob/114616d9f6ce9eba7c1aacd3d4a254f868010e25/sklearn/linear_model/_base.py#L306 The constructor stores data in the instance of the class which is used when fit is called.

Part of what is done in this function (

quantumMLdev/quantumml/mlModels.py

Line 62 in 4b02be3

def rebuild_SVR(query_results):

) rebuild_SVR also seems like it would be appropriate as an inherited class rather than a function, ie something like

class SVR(sklearn.svm.SVR):
    def __init__(query_results):
        super().__init__
        # extend class with what is in query results

I assume eventually this class will have the information that is the constructor as well as some associated model. like SVR?. I definitely see the parallels to models in scikitlearn, but the structure doesn't seem quite right yet.