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.

Sign up for GitHub

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

Jump to bottom

Filter observed values by ParameterProperties. #4678

Merged
merged 9 commits into from Apr 17, 2020
Merged

Filter observed values by ParameterProperties. #4678

merged 9 commits into from Apr 17, 2020

Conversation

geektoni
Copy link
Contributor

@geektoni geektoni commented Jun 9, 2019

No description provided.

@geektoni geektoni force-pushed the feature/observable_framework branch from 2b7b795 to b8cf6e0 Compare June 9, 2019 17:14
@karlnapf
Copy link
Member

karlnapf commented Jun 9, 2019

hard to see the diff now with all these commits

@geektoni geektoni force-pushed the feature/filter_by_property branch 3 times, most recently from a51f9b1 to b5e0690 Compare June 9, 2019 18:10
@geektoni
Copy link
Contributor Author

geektoni commented Jun 9, 2019

It should be much more readable now ;)

@geektoni geektoni marked this pull request as ready for review June 9, 2019 18:11
@geektoni geektoni force-pushed the feature/filter_by_property branch 3 times, most recently from db0ec6d to dfbba0c Compare June 9, 2019 18:36
karlnapf
karlnapf reviewed Jun 10, 2019
View reviewed changes
src/shogun/lib/observers/ParameterObserverScalar.h Outdated
const std::string& filename, std::vector<std::string>& parameters);
~CParameterObserverScalar();

CParameterObserverScalar(std::vector<std::string> &parameters);
Copy link
Member

Choose a reason for hiding this comment

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

do we even still need this class now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is still needed atm since it is used to serialize an observation into the Tensorboard format (which can be read by tensorboard --logdir lala).

@karlnapf
Copy link
Member

Please check this: if we filter and use ALL, then I explicitly include parameters with no properties (NONE) in clone. Might be good to do that here as well: https://github.com/shogun-toolbox/shogun/pull/4674/files#diff-9c3599c0d2090e493be261b079e9b63eR104

@geektoni geektoni changed the base branch from feature/observable_framework to develop November 4, 2019 16:14
@karlnapf
Copy link
Member

karlnapf commented Nov 4, 2019

updates here? :)

@geektoni
Copy link
Contributor Author

updates here? :)

Currently, I'm trying to rebase it and to finish it up! :)

@vigsterkr
Copy link
Member

@geektoni goodluckhavefun... sorry man but i didn't want to do a 4th rebase of the shared_ptr stuff :P

@geektoni
Copy link
Contributor Author

@geektoni goodluckhavefun... sorry man but i didn't want to do a 4th rebase of the shared_ptr stuff :P

💃 💃 💃

@geektoni
Copy link
Contributor Author

geektoni commented Apr 8, 2020

Please check this: if we filter and use ALL, then I explicitly include parameters with no properties (NONE) in clone. Might be good to do that here as well: https://github.com/shogun-toolbox/shogun/pull/4674/files#diff-9c3599c0d2090e493be261b079e9b63eR104

This behaviour is also respected by the ParameterObservers now.

@geektoni
Copy link
Contributor Author

geektoni commented Apr 8, 2020

@karlnapf This should be mostly done now.

@karlnapf
Copy link
Member

karlnapf commented Apr 9, 2020

Cool! What's next with this? Examples :) ?

@geektoni
Copy link
Contributor Author

geektoni commented Apr 9, 2020

Cool! What's next with this? Examples :) ?

Yup, at least one meta example to show how these things can be used in practice.

@gf712
Copy link
Member

gf712 commented Apr 9, 2020

Cool! What's next with this? Examples :) ?

Yup, at least one meta example to show how these things can be used in practice.

Would it be possible to extend a notebook with this? I think there is a tensorboard extension for notebooks.

@geektoni
Copy link
Contributor Author

geektoni commented Apr 9, 2020

Cool! What's next with this? Examples :) ?

Yup, at least one meta example to show how these things can be used in practice.

Would it be possible to extend a notebook with this? I think there is a tensorboard extension for notebooks.

Yes, it could be possible to extend a notebook with visualizations taken from the observers. As long as the underlying model used is observable of course.

@geektoni
Copy link
Contributor Author

geektoni commented Apr 9, 2020

This is ready to be merged once the CI approves.

karlnapf
karlnapf approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

nice that you picked this up! :)

@geektoni
Filter emitted values by looking at their ParameterProperties.
0818647 
* Add more constructors to the various ParameterObservers;
* Improve SGObject's subscribe() method to check also for properties;
* Refactor ParameterObserver unit tests;
* Change ParameterObservers class naming;
@geektoni
Change the license to the shorter version.
0cb89ec 
* Remove m_verbose from the ParameterObserverCV since we have a new logging
  facility now;
@geektoni
Round of clang-format to fix code style.
27818ae
@geektoni
Fix compilation errors within ParameterObserverCV.
3ed8109 
* Fix license header;
* Fix again code style;
@geektoni
Change license inside the ParameterObserver unit tests.
57fdb7f
@geektoni
Improve observation filtering by considering also the NONE property.
7027bb9 
If we are filtering the various values with a the ALL property, then
we explicity consider also the observations which have the NONE property.
@geektoni
Change the filter() implementation of ParameterObserver with STL.
82b4510 
* Use std::any_of() and empty();
* Run clang-format again;
@geektoni
Rename ParameterObserver filter() method to observes().
a3778a2 
This naming was preferred since filter() is assumed to return some
kind of subset of the given elements. This was not the case since
the method was returning a boolean.
@geektoni
Remove useless #include from ParameterObsever.cpp.
dcfc43e
@geektoni geektoni changed the title [WIP] Filter observed values by ParameterProperties. Filter observed values by ParameterProperties. Apr 15, 2020
@geektoni geektoni merged commit 261d2f6 into shogun-toolbox:develop Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karlnapf karlnapf karlnapf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@geektoni @karlnapf @vigsterkr @gf712