Fix support for printing arma objects with custom precision #895

Merged
merged 7 commits into from Mar 6, 2017

Conversation

Projects
None yet
4 participants
@shikharbhardwaj
Contributor

shikharbhardwaj commented Feb 28, 2017

Relevant issue : #755

A few issues :

  • After the user sets up custom flags, arma objects are printed with the set precision, but the stream does not align matrix elements.
  • During build, a pragma message appears

In file included from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/test_tools.hpp:15,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/mlpack_test.cpp:25:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:20:17: note: #pragma message: Armadillo was included before mlpack; this can sometimes cause prob
lems. It should only be necessary to include <mlpack/core.hpp> and not .
#pragma message "Armadillo was included before mlpack; this can sometimes cause\

because of the need to include arma in prefixedoutstream.hpp

@@ -19,7 +19,7 @@
#include <streambuf>
#include <stdexcept>
-#include <mlpack/core/util/sfinae_utility.hpp>
+#include <mlpack/core/arma_extend/arma_extend.hpp> // Includes Armadillo

This comment has been minimized.

@shikharbhardwaj

shikharbhardwaj Feb 28, 2017

Contributor

Includes arma to use is_arma_type, but causes the pragma message. Including prereqs.hpp or core.hpp causes the build to fail (I could not fully comprehend the reason, but it looked like a cyclic dependency). Build error

@shikharbhardwaj

shikharbhardwaj Feb 28, 2017

Contributor

Includes arma to use is_arma_type, but causes the pragma message. Including prereqs.hpp or core.hpp causes the build to fail (I could not fully comprehend the reason, but it looked like a cyclic dependency). Build error

This comment has been minimized.

@rcurtin

rcurtin Mar 3, 2017

Member

If there's no warning produced by this now, then this line is probably superfluous---Armadillo is being included somewhere earlier than this, so this line isn't needed.

@rcurtin

rcurtin Mar 3, 2017

Member

If there's no warning produced by this now, then this line is probably superfluous---Armadillo is being included somewhere earlier than this, so this line isn't needed.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 1, 2017

Member

Yes so we may need to change some ordering of includes in src/mlpack/prereqs.hpp so that Armadillo is included before prefixedoutstream.hpp. Basically, the ordering of includes is important because we graft some additional functionality onto Armadillo.

For the spacing issue, can't you set std::cellwidth (I think that is what it's called, I can't remember)? If that doesn't work then maybe some modification to Armadillo will be required, but that can be submitted upstream as a patch when done.

Member

rcurtin commented Mar 1, 2017

Yes so we may need to change some ordering of includes in src/mlpack/prereqs.hpp so that Armadillo is included before prefixedoutstream.hpp. Basically, the ordering of includes is important because we graft some additional functionality onto Armadillo.

For the spacing issue, can't you set std::cellwidth (I think that is what it's called, I can't remember)? If that doesn't work then maybe some modification to Armadillo will be required, but that can be submitted upstream as a patch when done.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 1, 2017

Contributor

For solving the spacing issue arma::print looks at the ranges of the elements to be printed, and figures out the type of stream flags to use from 4 predefined configurations.

In our use case, we try to print the elements with custom precision, we'd need to find the element which would take the maximum space in its integer part, and output each element with std::setw(max_int_width + padding), where padding is the required spacing. This requires accessing elements from the object.

Contributor

shikharbhardwaj commented Mar 1, 2017

For solving the spacing issue arma::print looks at the ranges of the elements to be printed, and figures out the type of stream flags to use from 4 predefined configurations.

In our use case, we try to print the elements with custom precision, we'd need to find the element which would take the maximum space in its integer part, and output each element with std::setw(max_int_width + padding), where padding is the required spacing. This requires accessing elements from the object.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 1, 2017

Member

It's your call on how to proceed here---remember that basically anything you do will be better than what we already have. :)

I wouldn't worry about doing operations on the input matrix/Armadillo object; if someone is printing a matrix, then the matrix is almost certainly going to be small (otherwise they will quickly realize that they, in fact, did not want to print the matrix!). So any computational overhead to, e.g., find the max will be negligible and probably outweighed by the overhead of printing itself.

Member

rcurtin commented Mar 1, 2017

It's your call on how to proceed here---remember that basically anything you do will be better than what we already have. :)

I wouldn't worry about doing operations on the input matrix/Armadillo object; if someone is printing a matrix, then the matrix is almost certainly going to be small (otherwise they will quickly realize that they, in fact, did not want to print the matrix!). So any computational overhead to, e.g., find the max will be negligible and probably outweighed by the overhead of printing itself.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 1, 2017

Contributor

I changed the code to use arma::arma_ostream::print, with the modify parameter as false, setting a custom width for the stream, calculating the cell width from the maximum value from the absolute values of the elements.

It worked as intended, but fails the test for printing the transpose of a Col, because an overload for arma::arma_ostream::print exists only for matrices, cubes and columns, not for arma::Op types.

Perhaps we can push a commit upstream to Armadillo with one such overload, which would basically be a wrapper unpacking the underlying type and calling the original function on it.

Here is the build error while compiling the test :

In file included from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream.hpp:162:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/log.hpp:18,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:98,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:15:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp: In instantiation of ‘typename std::enable_if<arma::is_arma_type::value, void>::type mlpack::util::PrefixedOutStream::BaseLogic(const T&) [with T = arma::Op<arma::Col, arma::op_htrans>; typename std::enable_if<arma::is_arma_type::value, void>::type = void]’:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp:32:15: required from ‘mlpack::util::PrefixedOutStream& mlpack::util::PrefixedOutStream::operator<<(const T&) [with T = arma::Op<arma::Col, arma::op_htrans>]’
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:85:20: required from here
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp:179:30: error: no matching function for call to ‘arma::arma_ostream::print(std::ostringstream&, const arma::Op<arma::Col, arma::op_htrans>&, bool)’
arma::arma_ostream::print(convert, val, false);
^
In file included from /usr/include/armadillo:361:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/arma_extend/arma_extend.hpp:52,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:90,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:15:
/usr/include/armadillo_bits/arma_ostream_meat.hpp:406:1: note: candidate: template static void arma::arma_ostream::print(std::ostream&, const arma::Mat&, bool)
arma_ostream::print(std::ostream& o, const Mat& m, const bool modify)
^
/usr/include/armadillo_bits/arma_ostream_meat.hpp:406:1: note: template argument deduction/substitution failed:
In file included from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream.hpp:162:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/log.hpp:18,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:98,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:15:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp:179:30: note: ‘const arma::Op<arma::Col, arma::op_htrans>’ is not derived from ‘const arma::Mat’
arma::arma_ostream::print(convert, val, false);

Thanks

Contributor

shikharbhardwaj commented Mar 1, 2017

I changed the code to use arma::arma_ostream::print, with the modify parameter as false, setting a custom width for the stream, calculating the cell width from the maximum value from the absolute values of the elements.

It worked as intended, but fails the test for printing the transpose of a Col, because an overload for arma::arma_ostream::print exists only for matrices, cubes and columns, not for arma::Op types.

Perhaps we can push a commit upstream to Armadillo with one such overload, which would basically be a wrapper unpacking the underlying type and calling the original function on it.

Here is the build error while compiling the test :

In file included from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream.hpp:162:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/log.hpp:18,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:98,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:15:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp: In instantiation of ‘typename std::enable_if<arma::is_arma_type::value, void>::type mlpack::util::PrefixedOutStream::BaseLogic(const T&) [with T = arma::Op<arma::Col, arma::op_htrans>; typename std::enable_if<arma::is_arma_type::value, void>::type = void]’:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp:32:15: required from ‘mlpack::util::PrefixedOutStream& mlpack::util::PrefixedOutStream::operator<<(const T&) [with T = arma::Op<arma::Col, arma::op_htrans>]’
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:85:20: required from here
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp:179:30: error: no matching function for call to ‘arma::arma_ostream::print(std::ostringstream&, const arma::Op<arma::Col, arma::op_htrans>&, bool)’
arma::arma_ostream::print(convert, val, false);
^
In file included from /usr/include/armadillo:361:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/arma_extend/arma_extend.hpp:52,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:90,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:15:
/usr/include/armadillo_bits/arma_ostream_meat.hpp:406:1: note: candidate: template static void arma::arma_ostream::print(std::ostream&, const arma::Mat&, bool)
arma_ostream::print(std::ostream& o, const Mat& m, const bool modify)
^
/usr/include/armadillo_bits/arma_ostream_meat.hpp:406:1: note: template argument deduction/substitution failed:
In file included from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream.hpp:162:0,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/log.hpp:18,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/prereqs.hpp:98,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core.hpp:217,
from /home/shikhar/Documents/dev/mlpack-fork/src/mlpack/tests/prefixedoutstream_test.cpp:15:
/home/shikhar/Documents/dev/mlpack-fork/src/mlpack/../mlpack/core/util/prefixedoutstream_impl.hpp:179:30: note: ‘const arma::Op<arma::Col, arma::op_htrans>’ is not derived from ‘const arma::Mat’
arma::arma_ostream::print(convert, val, false);

Thanks

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 1, 2017

Member

Ah, thanks for testing that rigorously. I think I know what we can do here.

You can use a little bit of template metaprogramming to catch when you have an Armadillo type that's an operation. You can use the arma::Proxy<> class for this, I think it will work pretty much everywhere. Take a look at armadillo_bits/Proxy.hpp to see what I mean. I think you could do this...

arma::Proxy<T> p;
// These hold printable objects inside... I think "Q" is always available.
p.Q.raw_print(...);
Member

rcurtin commented Mar 1, 2017

Ah, thanks for testing that rigorously. I think I know what we can do here.

You can use a little bit of template metaprogramming to catch when you have an Armadillo type that's an operation. You can use the arma::Proxy<> class for this, I think it will work pretty much everywhere. Take a look at armadillo_bits/Proxy.hpp to see what I mean. I think you could do this...

arma::Proxy<T> p;
// These hold printable objects inside... I think "Q" is always available.
p.Q.raw_print(...);
@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 1, 2017

Contributor

Thanks. That looks perfect.

Contributor

shikharbhardwaj commented Mar 1, 2017

Thanks. That looks perfect.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 2, 2017

Contributor

I ended up using arma::unwrap, as Proxy was not behaving well with arma::Op<arma::Mat<double>, arma::op_htrans>, when I tried to print a transposed matrix. I still understand only some of the template magic arma works on. :)

Contributor

shikharbhardwaj commented Mar 2, 2017

I ended up using arma::unwrap, as Proxy was not behaving well with arma::Op<arma::Mat<double>, arma::op_htrans>, when I tried to print a transposed matrix. I still understand only some of the template magic arma works on. :)

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 2, 2017

Member

Yeah, unwrap is fine, I was thinking that was the other way to do it.

Armadillo is crazy; what it does is very cool but figuring out what is actually going on is like a step into insanity thanks to the templates.

When this builds I'll review it and let you know.

Member

rcurtin commented Mar 2, 2017

Yeah, unwrap is fine, I was thinking that was the other way to do it.

Armadillo is crazy; what it does is very cool but figuring out what is actually going on is like a step into insanity thanks to the templates.

When this builds I'll review it and let you know.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 2, 2017

Contributor

This builds fine on my machine, with Armadillo 6.500.5. Some changes do exist in unwrap.hpp between the version used in Travis.

Contributor

shikharbhardwaj commented Mar 2, 2017

This builds fine on my machine, with Armadillo 6.500.5. Some changes do exist in unwrap.hpp between the version used in Travis.

@rcurtin

rcurtin approved these changes Mar 3, 2017

This looks great to me, thanks for the hard work. It will be nice to close the ticket and have nice serialization of Armadillo objects through the Log objects. If you'd like to handle the couple issues I pointed out, we can go ahead and merge. Also, if you'd like to add your name and email to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt, feel free. 👍

@@ -19,7 +19,7 @@
#include <streambuf>
#include <stdexcept>
-#include <mlpack/core/util/sfinae_utility.hpp>
+#include <mlpack/core/arma_extend/arma_extend.hpp> // Includes Armadillo

This comment has been minimized.

@rcurtin

rcurtin Mar 3, 2017

Member

If there's no warning produced by this now, then this line is probably superfluous---Armadillo is being included somewhere earlier than this, so this line isn't needed.

@rcurtin

rcurtin Mar 3, 2017

Member

If there's no warning produced by this now, then this line is probably superfluous---Armadillo is being included somewhere earlier than this, so this line isn't needed.

* @tparam T The type of the data to output.
* @param val The The data to be output.
*/
template<typename T>
- void BaseLogic(const T& val);
+ typename std::enable_if<!arma::is_arma_type<T>::value, void>::type

This comment has been minimized.

@rcurtin

rcurtin Mar 3, 2017

Member

No need for the second template parameter to enable_if---it's already default void.

@rcurtin

rcurtin Mar 3, 2017

Member

No need for the second template parameter to enable_if---it's already default void.

+ BOOST_REQUIRE_EQUAL(ss.str(),
+ BASH_GREEN "[INFO ] " BASH_CLEAR " 1.0000 1.5000 2.0000\n"
+ BASH_GREEN "[INFO ] " BASH_CLEAR " 2.5000 3.0000 3.5000\n"
+ BASH_GREEN "[INFO ] " BASH_CLEAR " 4.0000 4.5000 5.0000\n");

This comment has been minimized.

@rcurtin

rcurtin Mar 3, 2017

Member

These tests look great. If you don't mind, maybe you can split them into different tests? This causes some duplication since you have to recreate the Armadillo object each time, but it can make it easier to debug when something is going wrong, since you can quickly see that either one test fails or they all fail.

@rcurtin

rcurtin Mar 3, 2017

Member

These tests look great. If you don't mind, maybe you can split them into different tests? This causes some duplication since you have to recreate the Armadillo object each time, but it can make it easier to debug when something is going wrong, since you can quickly see that either one test fails or they all fail.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 3, 2017

Contributor

Thanks for looking into this.

We probably can't remove the include from prefixedoutstream.hpp, as it is required by log.hpp, which is included prereqs.hpp. Probably the reason why std headers are also left intact in this file. The build fails after removing the include. We could perhaps change the include to just #include <armadillo>, but I am not too sure about that as you mentioned some grafting happened with macros to add serialisation functionality to Armadillo objects.

I'll split the tests, make the other changes and commit ASAP.

I am already a contributor to mlpack! :D

Contributor

shikharbhardwaj commented Mar 3, 2017

Thanks for looking into this.

We probably can't remove the include from prefixedoutstream.hpp, as it is required by log.hpp, which is included prereqs.hpp. Probably the reason why std headers are also left intact in this file. The build fails after removing the include. We could perhaps change the include to just #include <armadillo>, but I am not too sure about that as you mentioned some grafting happened with macros to add serialisation functionality to Armadillo objects.

I'll split the tests, make the other changes and commit ASAP.

I am already a contributor to mlpack! :D

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 3, 2017

Member

Oh, I see, the grep command I ran on core.hpp was faulty then. I thought I remembered your contributions from before, so I thought it was strange that the grep produced nothing, but I think I forgot the case insensitive flag or something. Should have checked harder. :)

There could be strange subtle bugs introduced by including Armadillo at the wrong time, so I think that including arma_extend.hpp outside of prereqs.hpp (which has special setup before that include) could be problematic. It seems to me that by the time log.hpp is included in prereqs.hpp, arma_extend.hpp has already been included, so Armadillo symbols will already be available. Maybe the issue is that some other file included by prereqs.hpp is including log.hpp before arma_extend.hpp is included. I can look at it when I have a chance, but it may be some days before I have time, so if you'd like to dig in please feel free---I don't think it'll be difficult, just tedious.

Member

rcurtin commented Mar 3, 2017

Oh, I see, the grep command I ran on core.hpp was faulty then. I thought I remembered your contributions from before, so I thought it was strange that the grep produced nothing, but I think I forgot the case insensitive flag or something. Should have checked harder. :)

There could be strange subtle bugs introduced by including Armadillo at the wrong time, so I think that including arma_extend.hpp outside of prereqs.hpp (which has special setup before that include) could be problematic. It seems to me that by the time log.hpp is included in prereqs.hpp, arma_extend.hpp has already been included, so Armadillo symbols will already be available. Maybe the issue is that some other file included by prereqs.hpp is including log.hpp before arma_extend.hpp is included. I can look at it when I have a chance, but it may be some days before I have time, so if you'd like to dig in please feel free---I don't think it'll be difficult, just tedious.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 4, 2017

Contributor

@rcurtin I have made the changes and the PR is now merge ready. Thanks!

Contributor

shikharbhardwaj commented Mar 4, 2017

@rcurtin I have made the changes and the PR is now merge ready. Thanks!

@rcurtin rcurtin merged commit 47e872e into mlpack:master Mar 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 6, 2017

Member

Thanks. I made some minor style fixes in f463b88.

Member

rcurtin commented Mar 6, 2017

Thanks. I made some minor style fixes in f463b88.

@conradsnicta

This comment has been minimized.

Show comment
Hide comment
@conradsnicta

conradsnicta Mar 6, 2017

Contributor

This is a bad idea. You're using internal Armadillo structures (.P.Q) which are not guaranteed to be stable across Armadillo releases. Only the functions explicitly documented in http://arma.sourceforge.net/docs.html are guaranteed to be stable.

This is a bad idea. You're using internal Armadillo structures (.P.Q) which are not guaranteed to be stable across Armadillo releases. Only the functions explicitly documented in http://arma.sourceforge.net/docs.html are guaranteed to be stable.

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 6, 2017

Contributor

Thanks a lot for reviewing the code.
Is arma::unwrap also not intended to be used outside the library? I can think of an alternative way to write this, but can't think about an alternative to unwrap used in the lines above (line 149).

Contributor

shikharbhardwaj replied Mar 6, 2017

Thanks a lot for reviewing the code.
Is arma::unwrap also not intended to be used outside the library? I can think of an alternative way to write this, but can't think about an alternative to unwrap used in the lines above (line 149).

@conradsnicta

This comment has been minimized.

Show comment
Hide comment
@conradsnicta

conradsnicta Mar 6, 2017

Contributor

I've noticed that the code uses a whole bunch of Armadillo internals. This is not a good idea.

As per the Armadillo API Version Policy, the underlying internal implementation details may change across consecutive minor versions. Only the functions explicitly documented in http://arma.sourceforge.net/docs.html are guaranteed to be stable.

Contributor

conradsnicta commented Mar 6, 2017

I've noticed that the code uses a whole bunch of Armadillo internals. This is not a good idea.

As per the Armadillo API Version Policy, the underlying internal implementation details may change across consecutive minor versions. Only the functions explicitly documented in http://arma.sourceforge.net/docs.html are guaranteed to be stable.

@stereomatchingkiss

This comment has been minimized.

Show comment
Hide comment
@stereomatchingkiss

stereomatchingkiss Mar 6, 2017

Contributor

I've noticed that the code uses a whole bunch of Armadillo internals. This is not a good idea.

I agree it is not a good idea to use implementation details may change in the future, could you help us point out which part should not be used? I could open a pull request to fill up the gap, implement missing features. Thanks

Contributor

stereomatchingkiss commented Mar 6, 2017

I've noticed that the code uses a whole bunch of Armadillo internals. This is not a good idea.

I agree it is not a good idea to use implementation details may change in the future, could you help us point out which part should not be used? I could open a pull request to fill up the gap, implement missing features. Thanks

@conradsnicta

This comment has been minimized.

Show comment
Hide comment
@conradsnicta

conradsnicta Mar 6, 2017

Contributor

things like unwrap, Proxy, is_arma_type, arma_ostream should not be used. Essentially, if the functionality isn't described in http://arma.sourceforge.net/docs.html then it shouldn't be used.

Specific examples -- in the file https://github.com/mlpack/mlpack/blob/master/src/mlpack/core/util/prefixedoutstream_impl.hpp
line 38: typename std::enable_if<!arma::is_arma_type<T>::value>::type
line 146: typename std::enable_if<arma::is_arma_type<T>::value>::type
line 150: auto printVal = arma::unwrap<T>(val).M;
line 166: arma::arma_ostream::print(convert, printVal, true);
line 175: auto absVal = arma::abs(printVal).P.Q; (indirect use of internal Proxy class)
line 185: arma::arma_ostream::print(convert, printVal, false);

The code around line 146 can replaced with something along these lines:
template<typename T>
void
PrefixedOutStream::BaseLogic(const arma::Base<typename T::elem_type, T>& val)

Contributor

conradsnicta commented Mar 6, 2017

things like unwrap, Proxy, is_arma_type, arma_ostream should not be used. Essentially, if the functionality isn't described in http://arma.sourceforge.net/docs.html then it shouldn't be used.

Specific examples -- in the file https://github.com/mlpack/mlpack/blob/master/src/mlpack/core/util/prefixedoutstream_impl.hpp
line 38: typename std::enable_if<!arma::is_arma_type<T>::value>::type
line 146: typename std::enable_if<arma::is_arma_type<T>::value>::type
line 150: auto printVal = arma::unwrap<T>(val).M;
line 166: arma::arma_ostream::print(convert, printVal, true);
line 175: auto absVal = arma::abs(printVal).P.Q; (indirect use of internal Proxy class)
line 185: arma::arma_ostream::print(convert, printVal, false);

The code around line 146 can replaced with something along these lines:
template<typename T>
void
PrefixedOutStream::BaseLogic(const arma::Base<typename T::elem_type, T>& val)

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 6, 2017

Member

To some extent we can't get away from using the undocumented internal API. For what it's worth, even arma::Base<> is not actually documented in docs.html, so technically that would also be the internal API too. (Though I agree, arma::Base<> is certainly far less likely to change and I think it's a better solution.)

I think the auto usage is fine here (M will not be Op/Gen/anything that might confuse the compiler). If we don't use auto, we'll have to build some template metaprogramming struct to get the right type, because it could be Mat, Col, or Row depending. But, I agree, Armadillo+auto can be dangerous in a lot of situations. Do you have an alternative for what we are doing here?

I don't see a good way to escape using unwrap<> either; we need some way to take what we might have (which might be an expression of some form) and convert it to some block of memory that we can actually print. I can do something like Mat<T::elem_type> m = val but this incurs a copy; I'd like to avoid copies in general.

I also don't think we can escape using arma_ostream::print() for the purposes of this method, and I'm not excited about reimplementing that ourselves (which is the only realistic way I could see to avoid using that function).

In either case, I know that we are breaking the warranty and we get to pick up the pieces if something breaks or changes in later releases. :)

Member

rcurtin commented Mar 6, 2017

To some extent we can't get away from using the undocumented internal API. For what it's worth, even arma::Base<> is not actually documented in docs.html, so technically that would also be the internal API too. (Though I agree, arma::Base<> is certainly far less likely to change and I think it's a better solution.)

I think the auto usage is fine here (M will not be Op/Gen/anything that might confuse the compiler). If we don't use auto, we'll have to build some template metaprogramming struct to get the right type, because it could be Mat, Col, or Row depending. But, I agree, Armadillo+auto can be dangerous in a lot of situations. Do you have an alternative for what we are doing here?

I don't see a good way to escape using unwrap<> either; we need some way to take what we might have (which might be an expression of some form) and convert it to some block of memory that we can actually print. I can do something like Mat<T::elem_type> m = val but this incurs a copy; I'd like to avoid copies in general.

I also don't think we can escape using arma_ostream::print() for the purposes of this method, and I'm not excited about reimplementing that ourselves (which is the only realistic way I could see to avoid using that function).

In either case, I know that we are breaking the warranty and we get to pick up the pieces if something breaks or changes in later releases. :)

@conradsnicta

This comment has been minimized.

Show comment
Hide comment
@conradsnicta

conradsnicta Mar 6, 2017

Contributor

If I understand the rules for the auto keyword, the line auto printVal = arma::unwrap<T>(val).M; does a copy anyway. The line auto absVal = arma::abs(printVal).P.Q can lead to random crashes if printVal is an expression. At the very least I suggest using const auto& instead.

A betters solution would be using const Mat<typename T::elem_type>& printVal(val), which should avoid a copy if the val expression is simply a matrix. If val is a compound expression, the compiler will automatically evaluate the expression and stick the result into a temporary matrix (which we take a reference of using const Mat&). Code along these lines will lead to the avoidance of using the internal unwrap and Proxy classes.

As for using arma_ostream::print(), I believe using Mat::print(outputstream) or Mat::raw_print(outputstream) would be preferable. This also avoids the use of internal functions.

Also, I'm not sure it's worth worrying about potential extra copies, especially if this comes at the cost of breaking things later and an increased maintenance burden (ie. technical debt). The most time consuming part of many algorithms is matrix multiplication, not copies. "premature optimization is the root of all evil" and all that.

Contributor

conradsnicta commented Mar 6, 2017

If I understand the rules for the auto keyword, the line auto printVal = arma::unwrap<T>(val).M; does a copy anyway. The line auto absVal = arma::abs(printVal).P.Q can lead to random crashes if printVal is an expression. At the very least I suggest using const auto& instead.

A betters solution would be using const Mat<typename T::elem_type>& printVal(val), which should avoid a copy if the val expression is simply a matrix. If val is a compound expression, the compiler will automatically evaluate the expression and stick the result into a temporary matrix (which we take a reference of using const Mat&). Code along these lines will lead to the avoidance of using the internal unwrap and Proxy classes.

As for using arma_ostream::print(), I believe using Mat::print(outputstream) or Mat::raw_print(outputstream) would be preferable. This also avoids the use of internal functions.

Also, I'm not sure it's worth worrying about potential extra copies, especially if this comes at the cost of breaking things later and an increased maintenance burden (ie. technical debt). The most time consuming part of many algorithms is matrix multiplication, not copies. "premature optimization is the root of all evil" and all that.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Mar 6, 2017

Member

Ah, yeah, I agree, const auto& would be better, and the const Mat<typename T::elem_type>& is a better solution still. I think we should go with that one.

I think, but am not sure, that raw_print() could work for our situation here also; we could print that to a ostringstream and then send the ostringstream's contents to the Log object (which for whatever reason is not an ostream itself).

In this particular case, you are exactly right too, the copy doesn't matter. If a user is printing a matrix that's so large that a copy matters, well, they're already making a bad choice. However, I like to push for efficient code where possible regardless, so that new contributors or people reading the code can pick up good habits for fast code. It's easier to internalize "always avoid copies" instead of "avoid copies where it matters, and where it matters is something you have to think about", and that mindset also makes the maintenance burden easier for me when I evaluate PRs.

@shikharbhardwaj: are you interested in making these changes, or should I open another issue?

Member

rcurtin commented Mar 6, 2017

Ah, yeah, I agree, const auto& would be better, and the const Mat<typename T::elem_type>& is a better solution still. I think we should go with that one.

I think, but am not sure, that raw_print() could work for our situation here also; we could print that to a ostringstream and then send the ostringstream's contents to the Log object (which for whatever reason is not an ostream itself).

In this particular case, you are exactly right too, the copy doesn't matter. If a user is printing a matrix that's so large that a copy matters, well, they're already making a bad choice. However, I like to push for efficient code where possible regardless, so that new contributors or people reading the code can pick up good habits for fast code. It's easier to internalize "always avoid copies" instead of "avoid copies where it matters, and where it matters is something you have to think about", and that mindset also makes the maintenance burden easier for me when I evaluate PRs.

@shikharbhardwaj: are you interested in making these changes, or should I open another issue?

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Mar 6, 2017

Contributor

I tried to use const auto& to bind to the member from temporary from unwrap first, but as it turns out, doing so does not lead to a lifetime extension for the temporary object created, as noted here. Probably why the code segfaulted when I did use the const ref. Which means we'd need to bind to the temporary and access members afterwards.

@rcurtin I'll make the changes.

Contributor

shikharbhardwaj commented Mar 6, 2017

I tried to use const auto& to bind to the member from temporary from unwrap first, but as it turns out, doing so does not lead to a lifetime extension for the temporary object created, as noted here. Probably why the code segfaulted when I did use the const ref. Which means we'd need to bind to the temporary and access members afterwards.

@rcurtin I'll make the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment