NeuralNet's history API

[thomasjpfan]

Two API questions:

1. Should history be in __init__? This seems to be used for continue training. One would need to pass in an initialized module, criterion, optimizer, history, and set warm_start=True.
2. Should history be stored as history_?

[BenjaminBossan]

1. Should `history` be in `__init__`? This seems to be used for continue training.

I believe the main reason it is in __init__ is for some sklearn compatibility thing. If we wanted to allow passing a history object, we should change initialize_history. However, we already have load_params(f_history=...), maybe one way is enough.

2. Should `history` be stored as `history_`?

We thought about this at the beginning but decided that history is accessed often enough that the underscore could be annoying. I guess there's no harm in leaving it as is.

[thomasjpfan]

I believe the main reason it is in init is for some sklearn compatibility thing.

It is a little strange. Since initialize_history just creates a new History object, I can not see a use case for passing history into __init__.

We thought about this at the beginning but decided that history is accessed often enough that the underscore could be annoying. I guess there's no harm in leaving it as is.

There is no harm. It is very "scikit-learn" to have learnt attributes be stored with an underscore. I was considering adding a @property to define a history_ that returns `history. (or the other way around).

[BenjaminBossan]

It is a little strange. Since initialize_history just creates a new History object, I can not see a use case for passing history into __init__.

There is a check in sklearn's clone that parameters are consistent after cloning (I would add a link but github is down). This check can fail when history is not set during __init__.

With the naming of history vs history_, I think we should just leave it as is as long as it doesn't create any major problems.

[thomasjpfan]

There is a check in sklearn's clone that parameters are consistent after cloning

This could be because we are defined _get_param_names to be self.__dict__.keys() which includes all attributes.

Traditionally sklearn's clone creates a copy of the estimator with only its hyper-parameters and non of its learnt attributes.

With the naming of history vs history_, I think we should just leave it as is as long as it doesn't create any major problems.

I am okay with this.