Skip to content
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

Add factory for the ParameterObservers. #4557

Conversation

Projects
None yet
2 participants
@geektoni
Copy link
Contributor

commented Mar 4, 2019

This PR adds a factory for the parameter observers and it contains also some refactoring of the interface and the code itself (to make it consistent with the rest of Shogun).

  • Rename ParameterObserverInterface to ParameterObserver for consistency;
  • Make ParameterObserver inherith from SGObject;
  • Convert usages in the meta examples;
  • Split on_next into on_next and on_next_impl;
  • Add more methods to ParameverObserver to get a single observation;
  • Refactor ParameterObserverCV;
  • Fix SWIG interfaces;
  • Fix format;
@karlnapf
Copy link
Member

left a comment

Nice one!
I Made some initial comments. There are some conceptual issues that we should address first.
I’ll have a more detailed look in a bit, also will need to think a bit about how all the concepts interplay. Generally I think it’s a good idea to start from user code, I’ll write something down and get back
@gf712 do you also have some comments here?

Show resolved Hide resolved src/interfaces/swig/shogun.i Outdated
Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserver.cpp Outdated
Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserver.h Outdated

@geektoni geektoni force-pushed the geektoni:feature/parameter_observer_factory branch 5 times, most recently from c8fa32a to bf288cd Mar 13, 2019

@geektoni
Copy link
Contributor Author

left a comment

I've rebased onto #4552. Once the PR on refactoring ObservedValue is merged, this will become much more clear. Currently, I am facing some issues when I need to expose to SWIG Some<ObservedValue>.

Show resolved Hide resolved src/interfaces/swig/ParameterObserver.i Outdated

@geektoni geektoni force-pushed the geektoni:feature/parameter_observer_factory branch from bf288cd to f7c5665 Mar 14, 2019

@geektoni geektoni force-pushed the geektoni:feature/parameter_observer_factory branch from f7c5665 to 98f5313 Mar 18, 2019

@karlnapf

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Note convert is checking for overflows as well (some inline function)

@geektoni geektoni force-pushed the geektoni:feature/parameter_observer_factory branch 13 times, most recently from 76c6a9a to 5030cb8 Mar 21, 2019

@geektoni geektoni force-pushed the geektoni:feature/parameter_observer_factory branch 2 times, most recently from f8534ad to 4c83c24 Apr 1, 2019

geektoni added some commits Apr 4, 2019

Add unit tests to check the subscribe/unsubscribe methods.
* Fix again ParameterObserverCV tests;
Add a simple ParameterObserverLogger.
It will print on screen all the observed values.
Refactor CrossValidationStorage/FoldStorage such to be gettable.
* Fix memory error related to crossvalidation's fold results;
Remove specialized getter from the ParameterObserver interface.
* Fix meta examples;
* Fix static casting issue when unsubscribing an observer;
* Refactor all CrossValidationStorage and observers to use only CEvaluationResult
  as base class;
Refactor get(std::name, index) such to provide a non-throwing version.
It works more nicely with the interfaces and does not bloat the entire
terminal output with errors if it fails to cast to the corret type.

@geektoni geektoni force-pushed the shogun-toolbox:feature/observable_framework branch from 63c92d7 to 306a492 Apr 18, 2019

@geektoni geektoni force-pushed the geektoni:feature/parameter_observer_factory branch from ff62bd1 to ac2ef79 Apr 18, 2019

storage->set_num_runs(m_num_runs);
storage->set_num_folds(m_splitting_strategy->get_num_subsets());
storage->set_expose_labels(m_labels);
storage->put("num_runs", utils::safe_convert<index_t>(m_num_runs));

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 18, 2019

Member

minor: inside C++ code, I am not the biggest fan of using put with strings if the actual typed C++ API is available (setters). But not need to change that now

@geektoni geektoni merged commit 8512805 into shogun-toolbox:feature/observable_framework Apr 18, 2019

1 check passed

shogun-CI #20190418.6 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.