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

armadillo / bandicoot conv_to ambiguity #320

Closed
zoq opened this issue Sep 16, 2021 · 6 comments
Closed

armadillo / bandicoot conv_to ambiguity #320

zoq opened this issue Sep 16, 2021 · 6 comments

Comments

@zoq
Copy link
Member

zoq commented Sep 16, 2021

In some optimizers / functions we use conv_to<type>::from(X), now ADL doesn't work here since it matches arma::conv_to and coot::conv_to, so this will fail if I build against bandicoot. An easy solution is to introduce a type_trait for coot and arma types and implement a utility function that call's the right conv_to based on the type. But maybe there is a better solution? @conradsnicta @rcurtin

@conradsnicta
Copy link
Contributor

Dealing with ADL is always fun. Not sure what the best approach is without looking into this deeper. However, the suggested approach (essentially an abstraction layer) sounds pragmatic and doable.

(PS. Based on experience, ADL in its current state is a net negative in my view. In retrospect, its inclusion into the C++ standard was a mistake. A better solution would be to have an explicit using statement for every function/object that would be allowed to be pulled in during lookups and argument matching.)

@zoq
Copy link
Member Author

zoq commented Sep 17, 2021

I guess another solution would be to make conv_to a member function of X, instead of a standalone function. But maybe there is a reason why you opted out for this? So similar to X.max() I can also do X.conv_to<type>().

@conradsnicta
Copy link
Contributor

Armadillo aims to follow both the letter and spirit of Matlab syntax, where stand-alone functions take inputs and provide outputs, ie. Y = f(X). Member functions mainly aim to modify the underlying objects, eg. X.load().

There are targeted exceptions for pragmatic reasons, such as Y = X.t(), which aims to mimic the transpose operation in Matlab (ie. Y = X'). The X.max() member function is available so the maximum scalar value can be obtained regardless whether X is a vector or matrix, which is not possible in normal Matlab syntax without ugly workarounds that may have performance penalties (eg. val = max(max(X)), or val = max(vectorise(X))). It's of course possible to have dedicated internal code that ameliorates the performance penalties, but this adds to the maintenance burden and doesn't address the ugliness aspect.

For the conv_to family of functions, I'd rather not modify/extend the Armadillo API for a central function such as this. Adding the X.conv_to() member function would be inconsistent with the Y = f(X) pattern, and does not have a widely-applicable use case to warrant an exception. Furthermore, it takes ages for the Armadillo packages to be updated in various Linux distros, meaning it would take a long time for any new functionality to be sufficiently available. Case in point: ensmallen currently can only use Armadillo API up to version 9.800 due to Ubuntu 20.04 LTS.

@zoq
Copy link
Member Author

zoq commented Sep 19, 2021

Thanks for the clarification; I also don't want to backport specific armadillo functions because we like to support older versions. That said, I think the extra type-dependent conversion utility is good enough.

@rcurtin
Copy link
Member

rcurtin commented Sep 27, 2021

The workaround solution you proposed is a bit tedious, but unfortunately I don't think we can do much better than that. 👍

@mlpack-bot
Copy link

mlpack-bot bot commented Oct 27, 2021

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

@mlpack-bot mlpack-bot bot added the s: stale label Oct 27, 2021
@mlpack-bot mlpack-bot bot closed this as completed Nov 3, 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

3 participants