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

Create ObservedValue from AnyParameter and extend put() #4590

Conversation

Projects
None yet
3 participants
@geektoni
Copy link
Contributor

commented Mar 25, 2019

  • Add method to generate an ObservedValue from an AnyParameter;
  • Extend put() to emit also an ObservedValue;
Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@karlnapf I did some other experiments, but I am afraid that with the current design of the ObservedValue it is not possible to extend put() such to emit a value each time it is used.

@geektoni geektoni force-pushed the geektoni:feature/make_observed_value_from_tag branch 8 times, most recently from 56517cc to 0c37034 Mar 28, 2019

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

@geektoni geektoni force-pushed the geektoni:feature/make_observed_value_from_tag branch 7 times, most recently from 0c234d6 to 196053c Mar 29, 2019

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
Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@karlnapf I've updated the PR with the last changes. Let me know what do think about them. It should be good to go.

Once it is merged, I will update also #4592 and #4557

@karlnapf
Copy link
Member

left a comment

Nice, very nice! Glad this works now, well done on all the hard work with the templated observe calls ! :)

So one thing I dont agree with yet is the step parameter in put. You see, 90% of the time put is called, it is not used. This is why it is not a good idea to modify the API, as the 90% of the cases now need to deal with the parameter as well (and need to pass the parameter, and wonder what the default value of -1 means, etc, not very clean). So what I am saying is that the iteration number has to enter otherwise, either by a counter, or by the machine which knows the current iteration. I will think about this a bit now and might get back again. Any ideas on this so far?

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 force-pushed the geektoni:feature/make_observed_value_from_tag branch from 2a402af to 44b80c2 Mar 31, 2019

geektoni added some commits Apr 2, 2019

@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

I think this is ready to be merged. All CIs passed.

@vigsterkr

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

could the target branch of this be something like feature/observables ?

@karlnapf

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Could you make get_step protected? Then also no need to hide it from swig. Otherwise we can merge

@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Could you make get_step protected? Then also no need to hide it from swig. Otherwise we can merge

Sure!

@karlnapf

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

And I guess discuss with Viktor about develop vs feature branch. I’m good with both

SG_FORCED_INLINE int64_t get_step()
{
int64_t step = -1;
Tag<int64_t> tag("current_iteration");

This comment has been minimized.

Copy link
@vigsterkr

vigsterkr Apr 3, 2019

Member

this way of doing things will tie and observed value to be 'stepped' by current_iteration object tag... which should not be the case always. could be but not necessary

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 3, 2019

Member

This was preliminary.
Let’s make it virtual then!?

This comment has been minimized.

Copy link
@vigsterkr

vigsterkr Apr 3, 2019

Member

noup... it's the wrong place imo. discussed with @geektoni he'll try to address it later

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 4, 2019

Member

What’s the solution here then?

* Return a any version of the stored type.
* @return the any value.
*/
virtual Any& get_any()

This comment has been minimized.

Copy link
@vigsterkr

vigsterkr Apr 3, 2019

Member

what happens in swig?

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 3, 2019

Member

It’s simply not needed there

This comment has been minimized.

Copy link
@vigsterkr

vigsterkr Apr 3, 2019

Member

so how do i get the observed value in swig?

This comment has been minimized.

Copy link
@karlnapf

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 3, 2019

Member

That’s the whole point of this pr

This comment has been minimized.

Copy link
@vigsterkr

vigsterkr Apr 3, 2019

Member

this was already true prior pr, as observevalue is sgobject-like already... alas it wasn't registered... but imo it's really an unnecessary overhead to have this whole thing sgobject-like for the sake of registering and having the getter interface

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 3, 2019

Member

should have said series of pr. I agree I dont like the overhead as well. I would like to carve that functionality out of SGObject into a base class that just serves the parameter registration/access purposes. But this is of course out of scope for this little PR here.
The getter interface is a huge advantage as otherwise we once again need to expose/implement/swig-expose a lot of types. Let's discuss this next time on video

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
Show resolved Hide resolved src/shogun/base/SGObject.h Outdated
Show resolved Hide resolved src/shogun/base/SGObject.cpp 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 3, 2019

@geektoni geektoni force-pushed the geektoni:feature/make_observed_value_from_tag branch 2 times, most recently from bb7a5dd to fb8fc4f Apr 3, 2019

@geektoni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Once CI becomes green and if there are no objections I'll merge this in the feature branch.

@karlnapf
Copy link
Member

left a comment

mini comments, otherwise good from my side

Show resolved Hide resolved src/interfaces/swig/ParameterObserver.i
Show resolved Hide resolved src/shogun/base/SGObject.cpp Outdated

@geektoni geektoni force-pushed the geektoni:feature/make_observed_value_from_tag branch 2 times, most recently from e7af9de to 32977d8 Apr 3, 2019

@geektoni geektoni force-pushed the geektoni:feature/make_observed_value_from_tag branch from 32977d8 to b901539 Apr 3, 2019

* Get the current step for the observed values.
*/
#ifndef SWIG
SG_FORCED_INLINE int64_t get_step() const

This comment has been minimized.

Copy link
@geektoni

geektoni Apr 3, 2019

Author Contributor

@karlnapf If I do not guard it against SWIG this will generate an error when building the interfaces. I have no idea why since the method is protected. It may be caused by the fact that it is used inside put().

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 4, 2019

Member

Even if it is protected?

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 4, 2019

Member

Also what’s the error? :)

This comment has been minimized.

Copy link
@geektoni

geektoni Apr 4, 2019

Author Contributor
/build/opt/include/shogun/base/SGObject.h:994: Error: Syntax error - possibly a missing semicolon.
src/interfaces/python/CMakeFiles/_interface_python.dir/build.make:61: recipe for target 'src/interfaces/python/shogunPYTHON_wrap.cxx' failed
make[2]: *** [src/interfaces/python/shogunPYTHON_wrap.cxx] Error 1
CMakeFiles/Makefile2:86: recipe for target 'src/interfaces/python/CMakeFiles/_interface_python.dir/all' failed
make[1]: *** [src/interfaces/python/CMakeFiles/_interface_python.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

This comment has been minimized.

Copy link
@karlnapf

karlnapf Apr 5, 2019

Member

it is not protected. SWIG should ignore protected methods. If it is not possible to do that then guarding is fine

@karlnapf
Copy link
Member

left a comment

merge it into the feature branch!
But let’s make a note of the points that were discussed but not yet addressed

@geektoni geektoni merged commit a4ce20c into shogun-toolbox:feature/observable_framework Apr 4, 2019

1 check passed

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