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

Extend observe() to emit std::function objects #5033

Closed

Conversation

geektoni
Copy link
Contributor

@geektoni geektoni commented May 14, 2020

This addresses #5026. I still need to work around some details, but this is kind of what I had in mind to make it work. At the moment it doesn't pass the tests though. @karlnapf @gf712

this->watch_param(
name, &m_observed_value,
AnyParameterProperties(
"Unamed observed value", ParameterProperties::READONLY));
Copy link
Member

Choose a reason for hiding this comment

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

un-named? (also there is a type)

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'm not sure about this constructor. I may change it later on.

try
{
auto func =
value.first->get_any().as<std::function<std::shared_ptr<ObservedValue>()>>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gf712 I have a question here. Let's say my Any object contains an ObservedValueTemplated pointer object. The class ObservedValueTemplated inherits from ObservedValue. Ideally, I should be able to cast my Any to an ObservedValue pointer object right (e.g., any.as<std::shared_ptr<ObservedValue>>())?

I thought it would have been possible to do so thanks to the inheritance, but it does not seem to work :/ is this an intended behaviour (or am I missing something)?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it doesn't work off the bat. You need to register that inheritance using

shogun/src/shogun/lib/any.h

Lines 1259 to 1275 in 7014676

template <class From, class To>
void Any::register_caster(std::function<To(From)> caster)
{
const auto key = std::make_pair(
std::type_index{typeid(From)}, std::type_index{typeid(To)});
if (casting_registry.count(key))
{
return;
}
casting_registry[key] = [caster](void* src, void* dst) {
auto typed_src = static_cast<From*>(src);
auto typed_dst = static_cast<To*>(dst);
(*typed_dst) = caster(*typed_src);
};
}

Copy link
Member

Choose a reason for hiding this comment

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

This is basically a vtable, but it is only populated with SGObject derived classes to SGObject and some primitive types + std::string. I guess you could always register it there? @vigsterkr

* Add unit-tests;
* Fix code style;
return 20;
};

observe<std::function<int32_t()>>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, Any is smarter than I thought :) with the current implementation we can emit std::function directly without changing anything in the framework. We can also execute them in a fairly simple way by just calling get<lambda return type>("name of the lambda"). See the unit tests for an example.

Next question. Some of these operations could be quite costly and if the user calls them multiple times then they will be executed from scratch every time. Shall we somehow cache the result of these executions, so that the user will just need to call the lambda once and then it will retrieve only the pre-computed result?

This caching will happen only for the stored observations though.

Copy link
Member

Choose a reason for hiding this comment

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

could you make an example of such an operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you make an example of such an operation?

mmh I don't have any real example now. I was thinking about possible use cases/corner cases that we may face.

Copy link
Member

Choose a reason for hiding this comment

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

@geektoni I am modifying things a bit in #5037, so we use visitor patterns instead of any_cast. The only things you will have to keep in mind is that the visitor will only call a function when you get it if you have the ParameterProperties::FUNCTION, which is added in that PR. About the caching, you could use a static variable inside the lambda.

Copy link
Member

Choose a reason for hiding this comment

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

actually, maybe don't use static, that might cause massive issues

Copy link
Member

Choose a reason for hiding this comment

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

If you want to cache actually, use std::optional. Put the result there. If it is set it returns the value, otherwise computes it.

@stale
Copy link

stale bot commented Nov 15, 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 15, 2020
@stale
Copy link

stale bot commented Nov 22, 2020

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

@stale stale bot closed this Nov 22, 2020
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