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

Make classifiers observable. #4592

Conversation

Projects
None yet
2 participants
@geektoni
Copy link
Contributor

commented Mar 26, 2019

@karlnapf This is how it would look like with the current design. (Related to #4590)

@karlnapf
Copy link
Member

left a comment

Cool! Looks good already :)

The two main things from my side would be

  • use put in Perceptron and/or change the use-case to something where we need the custom (xvalidation)
  • maybe some API/filename cleanups so everything becomes a bit more brief (see inline comments)

@geektoni geektoni force-pushed the geektoni:feature/make_classifiers_observable branch 2 times, most recently from 4b2ec0a to 414d0a7 Apr 1, 2019

Show resolved Hide resolved src/shogun/classifier/Perceptron.cpp
@@ -89,7 +89,12 @@ void CAveragedPerceptron::iteration()
}
linalg::update_mean(w, cached_w, num_prev_weights);
linalg::update_mean(bias, cached_bias, num_prev_weights);

observe<SGVector<float64_t>>(get_step(), "w");

This comment has been minimized.

Copy link
@geektoni

geektoni Apr 1, 2019

Author Contributor

Regarding the AveragedPerceptron, I have used directly the observe method since we are using a linalg call to update both w and the bias. I thought it did not make any sense to use put() here since the values are updated in place (and it is more efficient). I think that in many other places around Shogun we will find these kinds of situations. What do you think? @karlnapf

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 1, 2019

Member

Yep I think that is good. As long as the algorithm is updating its members rather than some temporary variable.

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 1, 2019

Member

I am confused about the call, wouldnt you have to pass the w vector to the observe call? Or does this call mean "observe a model parameter 'w'" ? Because then why not just call put? What about the cases where I don't want to observe a model parameter, is there an observe call that accepts some value?
Sorry this might be obvious but I lost overview a bit :)

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 1, 2019

Member

One thing, the get_step() is not that nice here, maybe we could do that in the same way as in put? I.e. add inferring the step to the observe method? (We can keep the overloaded version that accepts a step, but I feel it would be verbose to do that in here). Also m_current_iteration actually visible at this point

This comment has been minimized.

Copy link
@geektoni

geektoni Apr 23, 2019

Author Contributor

@karlnapf I've updated these calls. Do you still have doubts about them? :)

Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
@karlnapf
Copy link
Member

left a comment

looks great, close to be merged I think :)

Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
Show resolved Hide resolved src/shogun/base/SGObject.h Outdated

@geektoni geektoni changed the base branch from develop to feature/observable_framework Apr 19, 2019

@geektoni geektoni force-pushed the geektoni:feature/make_classifiers_observable branch from 414d0a7 to cdf71d7 Apr 19, 2019

@geektoni geektoni changed the title [WIP] Make classifiers observable. Make classifiers observable. Apr 19, 2019

@geektoni geektoni force-pushed the geektoni:feature/make_classifiers_observable branch from cdf71d7 to f8b9d13 Apr 23, 2019

@geektoni geektoni merged commit 5941c23 into shogun-toolbox:feature/observable_framework Apr 23, 2019

1 check passed

shogun-CI #20190423.1 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.