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

[ShogunBoard] Add parameter observable and observe_param() method. #3877

Merged
merged 13 commits into from Jul 8, 2017

Conversation

Projects
None yet
2 participants
@geektoni
Contributor

geektoni commented Jun 27, 2017

The subscribe_to_parameters() method is still empty because I need to know
how ParameterObserver will be implemented @vigsterkr

@vigsterkr

This comment has been minimized.

Member

vigsterkr commented Jun 27, 2017

one way to define the interface is:

class ParameterObserver: public SGObject
{
public:
     ParameterObserver();
     virtual ~ParameterObserver();
     
     virtual void on_next(std::pair<std::string, Any>& ) = 0;
     virtual void on_error(std::exception_ptr) = 0;
     virtual void on_complete() = 0;
};

this way we avoid the template virtual hell :)

@@ -800,3 +806,17 @@ bool CSGObject::type_erased_has(const BaseTag& _tag) const
{
return self->has(_tag);
}
void CSGObject::subscribe_to_parameters(ParameterObserverInterface* obs)

This comment has been minimized.

@geektoni

geektoni Jun 28, 2017

Contributor

@vigsterkr I've added a simple ParameterObserverInterface class, as you suggested yesterday, to implement the behavior of subscribe_to_parameters().

geektoni added some commits Jun 23, 2017

[PrematureStopping] Register SIGINT signal handler.
The signal handler is set up when calling init_shogun(). It is
disabled by default (sg_signal->enable_handler() is needed).
Applied also style fixes.
[PrematureStopping] Add on_next(), on_pause() and on_complete() metho…
…d to CMachine.

The default behaviour of these methods is to set one of two variable called
m_cancel_computation or m_pause_computation. They can be used to terminate prematurely
algorithms or to pause them, by pressing CTRL+C.

Fixed also a bug inside lars_unittest.cc
[PrematureStopping] Use unique_ptr to wrap CSignal instance.
Replace also function pointers with std::function.
[PrematureStopping] Improve pause method using std::condition_variabl…
…e and locks.

Add COMPUTATION_CONTROLLERS macro to manage pause/cancel.
It will works also inside OpenMP environment.

Guarded from SWIG some methods which use SG_FORCED_INLINE.
[PrematureStopping] Convert CSignal::cancel_computations() to cancel_…
…computation().

Remove CSignal::cancel_computations() and add TODO when the class does not
inherit directly from CMachine.
[ShogunBoard] Add parameter observable and observe_scalar() method.
subscribe_to_parameters() is still empty because we need to know
how ParameterObserver will be implemented.

observe_scalar() takes now a pair <string, Any>, which are the name
of the observed parameter and the value (type erased) of the
parameter.
@geektoni

This comment has been minimized.

Contributor

geektoni commented Jun 30, 2017

@vigsterkr mmh it seems that some commits made with previous PRs disappeared from the shogun's branch feature/premature-stopping (that's why this PR is so crowded). Why did it happen? :/

@vigsterkr

This comment has been minimized.

Member

vigsterkr commented Jun 30, 2017

oooooooooooooooooooh fuck.... i think it might be my stupid mistake as i think i did the rebase on my local feature branch that was not synced with the latest upstream feature branch :((((

# TFLogger package
FIND_PACKAGE(TFLogger 0.1.0 CONFIG)
IF (TFLogger_FOUND)
SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM ${TFLogger_INCLUDE_DIR})

This comment has been minimized.

@vigsterkr

vigsterkr Jun 30, 2017

Member

do we need really public system? as this is really gonna be just statically linked into .so... so afaik PRIVATE should be enough, no?

FIND_PACKAGE(TFLogger 0.1.0 CONFIG)
IF (TFLogger_FOUND)
SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM ${TFLogger_INCLUDE_DIR})
target_link_libraries(shogun PUBLIC tflogger::tflogger)

This comment has been minimized.

@vigsterkr

vigsterkr Jun 30, 2017

Member

same here i reckon a private would be enough...

@geektoni

This comment has been minimized.

geektoni added some commits Jun 28, 2017

[ShogunBoard] Add a ParameterObserver (interface and implementation) …
…and add TBOutputFormat.

Add interface and ParameterObserverScalar which are the classes needed to
add parameter watchers to an algorithm. Add also TBOutputFormat class
which converts an ObservedValue into a tensorflow::Event object.

Other:
* Add unit tests (ParameterObserverScalar and TBOutputFormat);
* Add CMake switch to find TFLogger (or download and build it if not present);
* Add SWIG code to make ParameterObserver visible to interfaces;
[ShogunBoard] Add ParameterObserverHistogram which generate a TB's hi…
…stogram.

Other:
* Add convert_vector() to TBOutputFormat;
* Add TBOutputFormat convert_vector() unit tests;
* Add TF histogram.h/.cc helper classes;
if (m_parameters.size() == 0)
return true;
for (auto v : m_parameters)

This comment has been minimized.

@vigsterkr

vigsterkr Jul 7, 2017

Member

indentation ?

ParameterObserverScalar::~ParameterObserverScalar()
{
m_writer.flush();
m_writer.close();

This comment has been minimized.

@vigsterkr

vigsterkr Jul 7, 2017

Member

indentation

/**
* Writer object which will be used to write tensorflow::Event files
*/
tflogger::TensorFlowLogger m_writer;

This comment has been minimized.

@vigsterkr

vigsterkr Jul 7, 2017

Member

mmm i'm not so sure if an interface (abstract class) should actually contain this

geektoni added some commits Jul 7, 2017

[ShogunBoard] Refactor ParameterObserverInterface to make it pure.
Add a ParameterObserverTensorBoard class which implements the
TFLogger utilities. Run also clang-format and add the new classes
to SWIG.
[ShogunBoard] Guard everything TFLogger related.
Add a HAVE_TFLOGGER flag.
#include <float.h>
#include <math.h>
#include <vector>
#include <tflogger/summary.pb.h>

This comment has been minimized.

@vigsterkr

vigsterkr Jul 7, 2017

Member

this implementation and the whole tfhistogram needs guard of HAVE_TFLOGGER

[ShogunBoard] Add BSD-3 license.
Other:
* Refactor TBOutputFormat unit test;
* Guard with HAVE_TFLOGGER unit tests and Tensorflow histogram files;
* Apply clang-format to some files (gaurded histogram.cpp/.h with clang-format off);
@vigsterkr

This comment has been minimized.

Member

vigsterkr commented Jul 8, 2017

mmm seems the changes really affect the compile time, i'm merging and we'll try to figure out in the feature branch how to solve this...

@vigsterkr vigsterkr merged commit d607286 into shogun-toolbox:feature/premature-stopping Jul 8, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment