Improve cancel_computation to enable testing. #4293
Conversation
| EXPECT_TRUE(a.get_check() == 5); | ||
| } | ||
|
|
||
| TEST(StoppableSGObject, custom_callback_by_user) |
There was a problem hiding this comment.
This test shows more clearly what we are going to do in the future when testing the premature stopping feature.
src/shogun/lib/StoppableSGObject.cpp
Outdated
| @@ -40,6 +40,12 @@ rxcpp::subscription CStoppableSGObject::connect_to_signal_handler() | |||
| return get_global_signal()->get_observable()->subscribe(subscriber); | |||
| } | |||
|
|
|||
| void CStoppableSGObject::add_callback(std::function<bool()> callback=nullptr) | |||
There was a problem hiding this comment.
add suggests to add them to a stack of callbacks
| }; | ||
|
|
||
|
|
||
| Mock_model a; |
There was a problem hiding this comment.
So how would this be tested with existing machines?
There was a problem hiding this comment.
Ok I guess your idea here is that we only test things for the Mock_model once and not again for all the subsequent machines?
| } | ||
| return true; | ||
| } | ||
| int m_check; |
There was a problem hiding this comment.
maybe those two can be renamed to something more descriptive
| return false; | ||
| }; | ||
|
|
||
| // We then add the callback |
| // Custom train machine | ||
| virtual bool train_machine(CFeatures* data=NULL) | ||
| { | ||
| for (int k=0; k<10; k++) |
There was a problem hiding this comment.
could we rename k to something that indicates it is the "train-loop" counter.
Like "num_iterations_train"
| using namespace shogun; | ||
| using namespace std; | ||
|
|
||
| class Mock_model : public CMachine |
geektoni
force-pushed
the
feature/callback_cancel_computation
branch
from
60d08f1
to
2d4c790
Compare
src/shogun/lib/StoppableSGObject.h
Outdated
| if (m_callback) | ||
| result_callback = m_callback(); | ||
|
|
||
| return m_cancel_computation.load() || result_callback; |
There was a problem hiding this comment.
since it's || maybe it'd be better to have this as:
return (m_callback) ? (m_cancel_computation.load() || result_callback()) : m_cancel_computation.load();
just for the sake of lazy evaluation of or :)
There was a problem hiding this comment.
It seems a good idea ;)
| TEST(StoppableSGObject, empty_callback) | ||
| { | ||
| MockModel a; | ||
| a.set_callback(nullptr); |
There was a problem hiding this comment.
you either use the default (nullptr) valued set_callback, i.e. use
a.set_callback();
or remove the default value of the function arg. i would opt for the latter
|
|
||
| virtual const char* get_name() const | ||
| { | ||
| return "Mock_model"; |
@karlnapf @shubham808
This PR contains the first implementation of the new
cancel_computationmethod. It is still a WIP so any comments on it are appreciated. Please see the unit test to see the new behaviour.