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

Refactor ObservedValue. #4552

Merged

Conversation

Projects
None yet
3 participants
@geektoni
Copy link
Contributor

commented Mar 2, 2019

Clean up the ObservedValue class from useless enums since the Any conversion will be done by the observers with dynamic dispatching.

@karlnapf
Copy link
Member

left a comment

You updated data by accident I think (needs change)

Otherwise some more minor comments...
Nice cleanup! I wonder whether the general structure of the observer classes can even be simplified?

Show resolved Hide resolved src/shogun/base/SGObject.cpp
Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserverCV.cpp Outdated

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch from d0b481e to 04cdb4f Mar 5, 2019

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

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch from 91f87d0 to 5e7c5e3 Mar 7, 2019

@geektoni
Copy link
Contributor Author

left a comment

@karlnapf Splitting the ObservedValue into a templated version seems to work just fine. I've just hit a couple of issues which I think are no big deal. See inline comments.

Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValue.cpp Outdated
Show resolved Hide resolved src/shogun/io/TBOutputFormat.cpp
Show resolved Hide resolved tests/unit/io/TBOutputFormat_unittest.cc Outdated
@karlnapf
Copy link
Member

left a comment

Cool! This seems to work then which is a great exploit of the runtime parameter framework. I made a few comments and once this is iterated I think we made some progress! :)

Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
Show resolved Hide resolved src/shogun/base/SGObject.cpp
Show resolved Hide resolved src/shogun/io/TBOutputFormat.cpp
Show resolved Hide resolved src/shogun/io/TBOutputFormat.cpp
Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValue.h Outdated
Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserverCV.cpp
Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValueTemplated.h Outdated
Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValueTemplated.h Outdated
Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValueTemplated.h Outdated
~ObservedValueTemplated() {};

Any get_any() {
return make_any(v);

This comment has been minimized.

Copy link
@karlnapf

karlnapf Mar 8, 2019

Member

What about storing the any and returning a reference? Not sure about overhead ...

This comment has been minimized.

Copy link
@geektoni

geektoni Mar 11, 2019

Author Contributor

This is more of a convenience method to make some things works. I'm trying to see if I can remove it somehow :/

This comment has been minimized.

Copy link
@geektoni

geektoni Mar 12, 2019

Author Contributor

I guess this method will need to stay here for a bit longer. The only place where is needed is here https://github.com/shogun-toolbox/shogun/pull/4552/files#diff-d4ce27ca628f4ba5cacbb66e265af3d0L114, but it is not completely trivial to remove it.

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch 3 times, most recently from f59413f to 932b882 Mar 11, 2019

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch 2 times, most recently from 93e8047 to 3338b1f Mar 12, 2019

@geektoni geektoni changed the title [WIP] Remove enum from ObservedValue. [WIP] Refactor ObservedValue. Mar 12, 2019

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch from d0ffcfc to 875926a Mar 12, 2019

@geektoni geektoni changed the title [WIP] Refactor ObservedValue. Refactor ObservedValue. Mar 12, 2019

@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@karlnapf Apart from some minor stuff, I think this guy here is 90% complete.

@karlnapf
Copy link
Member

left a comment

I made some more nitpicking comments now that the high level stuff is in place. Style stuff and some minor design things. I think this is mergable pretty soon. Nice! :)

Show resolved Hide resolved src/shogun/base/SGObject.cpp Outdated
Show resolved Hide resolved src/shogun/evaluation/CrossValidation.cpp
Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValue.cpp Outdated
{
m_type = type;
SG_NOTIMPLEMENTED
return make_any(nullptr);

This comment has been minimized.

Copy link
@karlnapf

karlnapf Mar 14, 2019

Member

Did you check the overhead of this

@lisitsyn thought?

Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValueTemplated.h Outdated
Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserverCV.cpp
Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserverCV.cpp Outdated
@karlnapf

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

It depends a bit: if you only ever use the size to iterate over indices used for a std vector it is fine. If the indices are used in any other shogun data structure, it won’t be
Why don’t we just introduce a conversion tool that can be used in other places as well.... the problem popped up before in the testing framework

Show resolved Hide resolved src/shogun/util/utils.h Outdated

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch 2 times, most recently from 2e4b25a to 2d78083 Mar 19, 2019

@karlnapf

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Flying now. Will check in detail later today

@karlnapf
Copy link
Member

left a comment

I might have asked this before but forgot: why not store the any in the class? Isn’t there overhead?

@karlnapf
Copy link
Member

left a comment

I think this is ready :)
If we can have somebody else check it that’d be great, otherwise I’ll merge it later when landed
Really nice improvement now imo

@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I might have asked this before but forgot: why not store the any in the class? Isn’t there overhead?

No, there should not be too much overhead. I think that it will just save us from calling make_any when we need the untyped value. I do not remember either which usage we had in mind such to make us keep also the any :P

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch from 2d78083 to ff79e6a Mar 19, 2019

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch from 7f5c2b3 to f295ae5 Mar 19, 2019

@@ -81,6 +82,8 @@ namespace shogun
int64_t m_step;
/** Parameter's name */
std::string m_name;
/** Untyped value */
Any m_any_value;

This comment has been minimized.

Copy link
@geektoni

geektoni Mar 19, 2019

Author Contributor

@karlnapf This way we will store directly also the Any reference (I don't know why the formatting is messed up :/).

This comment has been minimized.

Copy link
@karlnapf

karlnapf Mar 19, 2019

Member

Store value or reference?

This comment has been minimized.

Copy link
@geektoni

geektoni Mar 20, 2019

Author Contributor

I would say by value because it is more self-contained.

@karlnapf
Copy link
Member

left a comment

My comments are getting smaller and smaller. I will wait for your responses and then I think we are good!

Show resolved Hide resolved src/shogun/lib/parameter_observers/ObservedValue.h Outdated
{
m_type = type;
return m_any_value;

This comment has been minimized.

Copy link
@karlnapf

karlnapf Mar 19, 2019

Member

return value or reference?

This comment has been minimized.

Copy link
@geektoni

geektoni Mar 20, 2019

Author Contributor

I guess that return by reference would be okay.

@@ -81,6 +82,8 @@ namespace shogun
int64_t m_step;
/** Parameter's name */
std::string m_name;
/** Untyped value */
Any m_any_value;

This comment has been minimized.

Copy link
@karlnapf

karlnapf Mar 19, 2019

Member

Store value or reference?

Show resolved Hide resolved src/shogun/lib/parameter_observers/ParameterObserverCV.cpp Outdated
@karlnapf
Copy link
Member

left a comment

I think this is now done :)

@karlnapf

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Alright, before we merge, the CI errors needs to be addressed ...

Fix bug when initializing ObservedValue.
* Update unit-test to check also for Any.
@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Okay, it should be good now. Let's see if CI likes it.

@geektoni geektoni force-pushed the geektoni:feature/remove_enum_observed_value branch from a9b9595 to e601ae3 Mar 21, 2019

@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@karlnapf Should be okay now? :)

"value", &m_observed_value,
AnyParameterProperties(
description, ParameterProperties::NONE));
m_any_value = make_any(m_observed_value);

This comment has been minimized.

Copy link
@karlnapf

karlnapf Mar 21, 2019

Member

whitespace is weird here

This comment has been minimized.

Copy link
@geektoni

geektoni Mar 21, 2019

Author Contributor

I'll do a round of code formatter

@karlnapf karlnapf merged commit 11f8fcb into shogun-toolbox:develop Mar 21, 2019

1 check was pending

shogun-CI in progress
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.