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

[WIP] Make Random Forest observable. #4655

Closed

Conversation

geektoni
Copy link
Contributor

No description provided.

@geektoni geektoni force-pushed the feature/observable_randomforest branch 5 times, most recently from fe8419a to 9068b55 Compare May 31, 2019 08:57
@@ -12,6 +12,14 @@ namespace shogun {
MOCK_METHOD0(get_values, SGVector<float64_t>());

virtual const char* get_name() const { return "MockCLabels"; }

protected:
virtual CSGObject *create_empty() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont understand why you need this as there is never an empty instance created ... or am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were situations in which we were emitting MockLabels from a machine and therefore I needed to implement the create_empty() method to make clone() work.

@geektoni geektoni force-pushed the feature/observable_framework branch from d21f741 to bac22e6 Compare June 3, 2019 12:17
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 9068b55 to cecc5df Compare June 3, 2019 12:32
geektoni added a commit to geektoni/shogun that referenced this pull request Jun 3, 2019
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 64e3ab7 to fdf4aa0 Compare June 3, 2019 12:58
geektoni added a commit to geektoni/shogun that referenced this pull request Jun 3, 2019
@geektoni geektoni force-pushed the feature/observable_randomforest branch from fdf4aa0 to 7320afe Compare June 3, 2019 13:02
geektoni added a commit to geektoni/shogun that referenced this pull request Jun 3, 2019
* Round of clang format;
* Remove CombinationRule casting inside BagginMachine unit tests.
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 7320afe to 17d9464 Compare June 3, 2019 13:40
geektoni added a commit to geektoni/shogun that referenced this pull request Jun 3, 2019
* Round of clang format;
* Remove CombinationRule casting inside BagginMachine unit tests.
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 7eea42f to e9113af Compare June 8, 2019 13:25
geektoni added a commit to geektoni/shogun that referenced this pull request Jun 8, 2019
* Round of clang format;
* Remove CombinationRule casting inside BagginMachine unit tests.
@geektoni geektoni force-pushed the feature/observable_randomforest branch from e9113af to 18fcded Compare June 8, 2019 14:47
@geektoni geektoni force-pushed the feature/observable_framework branch 2 times, most recently from 2b7b795 to b8cf6e0 Compare June 9, 2019 17:14
@stale
Copy link

stale bot commented Feb 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@geektoni
Copy link
Contributor Author

geektoni commented Apr 8, 2020

@geektoni is this still needed?

I'm picking it up again these days. Let's see after I fix all these merge conflicts.

geektoni added a commit to geektoni/shogun that referenced this pull request Apr 9, 2020
* Round of clang format;
* Remove CombinationRule casting inside BagginMachine unit tests.
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 18fcded to 9a4ed4b Compare April 9, 2020 15:07
@geektoni geektoni changed the base branch from feature/observable_framework to develop April 9, 2020 15:08
geektoni added 9 commits May 3, 2020 17:57
* Fix CBaggingMachine unit tests by using the put()/get() methods;
* Fix RandomForest constructors (remove missing methods);
* Fix RandomForest unit tests;
It can be called now without passing an explicit CEvaluation object.
This will be generated automatically, given the features' type, and
it will be used to compute the oob error.
It will emit oob error during training and each of the trained machines.
* Add CCombinationRule to base_types.h;
* Add missing SG_REF()/SG_UNREF() pair.
These are added to enable put() semantic.

* Fix random forest meta examples;
* Round of clang format;
* Remove CombinationRule casting inside BagginMachine unit tests.
* Removed some observable code until the get_oob_error() gets fixed
(or updated);
* observe() now accept a const reference to the ObservedValue;
* BaggingMachine should be better observable;
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 9a4ed4b to da1aa6d Compare May 3, 2020 16:08
geektoni added 2 commits May 4, 2020 21:07
* Fix RandomForest unit test (casting error);
* Remove compute_oob observable;
* Remove Random Forest meta example (they were probably left there when
rebasing against develop);
* Fix code style;
The OOB error is computed only if the combination rule and the evaluation
metric are specified. Moreover, there must be observers attached to the
machine.
@geektoni geektoni force-pushed the feature/observable_randomforest branch from 6eb4e3a to 9dfa8e4 Compare May 5, 2020 08:37
if (this->m_combination_rule && this->m_oob_evaluation_metric &&
this->get_num_subscriptions() != 0)
{
auto oob_error = this->get_oob_error();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: you now always compute the oob error whenever there is an observer ...Is that good? I forgot how the specifying to be observed variables works, but can't the user somehow pick what she wants to observe? In that case this should only be computed if the user requested it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the idea is that as long as the model has some observer attached, then it will output all the available observations. This means that the OOB error will be computed regardless of the user preferences.

It will be the observer job to filter the observations and keep just the ones that the user wants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, there is no way to tell an observable model to emit only certain observations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that’s good and we should probably think about a way to avoid this. Passing (and ignoring) pointers to existing data is one thing, but expensive operations should only be executed if desired.
I could imagine that this could be done with a lambda function that is only executed if the observer actually wants (as in user instructed) to store the data. Maybe @gf712 has an idea he is the lambda master?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the problem. One small issue here is that the model does not have any idea about which things the observers want to observe. I guess the solution could be emitting functions (lambdas) instead which can then be executed by the observers if they want to store that kind of data. Is this what you meant @karlnapf?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the model doesnt know. In general, it should offer the observer to observe something (which the observer then can or cannot do, depending on its settings). Passing a pointer is a good example for that. The observer can clone the underlying structure for example.
But for costly things, passing a lambda that would compute something expensive is a better choice imo, so yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense! I will do some experimenting to try adding this lambda feature. I'm leaving this one as it is (or maybe we could merge it in a feature branch) and then I will open a new PR with the lambda observers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say let's just keep this PR open, and then you could rebase it once the the lambda thing works. Alternatively, just remove the oob error stuff and we can merge everything else?

@stale
Copy link

stale bot commented Nov 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 5, 2020
@stale
Copy link

stale bot commented Nov 12, 2020

This issue is now being closed due to a lack of activity. Feel free to reopen it.

@stale stale bot closed this Nov 12, 2020
@gf712 gf712 removed the stale label Dec 10, 2020
@gf712 gf712 reopened this Dec 10, 2020
@stale
Copy link

stale bot commented Jun 9, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 9, 2021
@stale
Copy link

stale bot commented Jun 16, 2021

This issue is now being closed due to a lack of activity. Feel free to reopen it.

@stale stale bot closed this Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants