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

Chain SGObject::put #5047

Open
gf712 opened this issue May 28, 2020 · 7 comments
Open

Chain SGObject::put #5047

gf712 opened this issue May 28, 2020 · 7 comments

Comments

@gf712
Copy link
Member

gf712 commented May 28, 2020

I was wondering if it would make sense for SGObject::put to return this by reference? That way we could chain various put calls:

auto k = create<Kernel>("GaussianKernel")
         .put("log_width, 1.)
         .put("cache_size", 200);
@karlnapf
Copy link
Member

yep I think that would be nice

@gf712 gf712 added this to the Shogun 7.0.0 milestone Jun 4, 2020
@mr3543
Copy link

mr3543 commented Jun 16, 2020

hey I was having a look at this issue and thought I might be able to help. I was under the impression create<Kernel>("GaussianKernel") would return a std::shared_ptr<Kernel> so chaining the put calls would have to look something like

auto k = create<Kernel>("GaussianKernel") ->put("log_width",1) ->put("cache_size",200);

and put would have to return a shared_ptr. I just started looking at this repo though so I could be off here.

@gf712
Copy link
Member Author

gf712 commented Jun 16, 2020

Yup, that’s correct :) so you would probably have to return something like shared_from_this() in put

@mr3543
Copy link

mr3543 commented Jun 16, 2020

cool, thanks!

@mr3543
Copy link

mr3543 commented Jun 16, 2020

I tried making the change but there are a few test cases (358,432,443,470,476) that make objects without shared pointers and call put. These throw a bad_weak_ptr exception, see https://stackoverflow.com/questions/27697973/shared-from-this-causing-bad-weak-ptr.

I can try to change the instantiation in these test cases, but wasn't sure if that's something the team is ok with.

@gf712
Copy link
Member Author

gf712 commented Jun 16, 2020

Hmm, try to get it to run and send a PR and then we can have a look :)

@jonpsy
Copy link
Contributor

jonpsy commented Jan 5, 2021

Smells like #5139. I'll add this to my list. Although, I'm a bit worried about its implications in SWIG interfaces.

@jonpsy jonpsy mentioned this issue Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants