These are chat archives for symengine/symengine

24th
Jun 2015
Ondřej Čertík
@certik
Jun 24 2015 02:41 UTC
So what was the problem?
@Sumith1896 can you provide exact instructions how to run your latest benchmarks on my machine? (which branch, which executable)
If Piranha is 13ms and we are 14-15ms, that I think is close enough. Obviously there is still some difference, so it'd be nice to understand what is causing it, but if we can get to 14ms, I am ok with that.
Now is time to start thinking how to best implement this in SymEngine. @bluescarni do you have any ideas about the design? For example, would you recommend to just depend on Piranha? That might be the best, as long as Piranha can be installed on all platforms (like in Sage on OSX ...)
Then we should use the building blocks from Piranha to implement the polynomials in symengine, or perhaps even use Piranha's polynomials themselves. @bluescarni what do you think?
Ondřej Čertík
@certik
Jun 24 2015 04:29 UTC
@Sumith1896 also, while you remember it, can you write up into your wiki report all the things that improved performance, and also all the things you tried that didn't improve the performance? So that we have it written up for the future.
Isuru Fernando
@isuruf
Jun 24 2015 08:21 UTC
@abinashmeher, some comments about ruby wrappers.
  • Do we need Manifest.txt?
  • In your gemspec file, specify the files explicitly (not like lib/**/*, because it would include cmake files as well)
  • In gemspec file, not all files like basic_spec.rb files are not added to the gem.
  • Add support for out of tree builds
  • Move lib/symengine/CMakeLists.txt to ext/symengine/
Sumith Kulal
@Sumith1896
Jun 24 2015 08:35 UTC
Yes, we have to worry about Piranha builds across platforms before having it as a hard dependency.
I'll update the wiki report, forgot to do it late night yesterday.
Sumith Kulal
@Sumith1896
Jun 24 2015 11:07 UTC
Also I'll have to look into the slowdown of benchmarks I had previously reported.
Sumith Kulal
@Sumith1896
Jun 24 2015 11:40 UTC
@certik you can run the expand2d in sympy/symengine#470
to try out the benchmark in your machine
Isuru Fernando
@isuruf
Jun 24 2015 11:54 UTC
@Sumith1896, why do you need -DNDEBUG? Can you post the full output of cmake command
Francesco Biscani
@bluescarni
Jun 24 2015 11:56 UTC
@certik I see no reason why Piranha should not work in OSX, but I have not tried it yet (I am not really a Mac guy). I might be able to get ssh access to an OSX box though, so I should be able to make sure that everything works as expected
regarding the licensing, I have decided I will do as GMP - dual license GPL/LGPL
I haven't gotten around changing the headers and the readme yet, but I will soon
expanding on the portability thing... back in May I did compile and used Piranha in MinGW for a while (I was stuck on a Windows machine for a while)
but MSVC compatibility will not happen at the very least for a year
MSVC is too far behind in supporting C++11
that was a MinGW 64 bit compilation by the way
Sumith Kulal
@Sumith1896
Jun 24 2015 11:59 UTC
@isuruf I think because Piranha release build is triggered that way.
We got good speed up
Maybe some other libraries also use it that way, hence we can get extra performance
Francesco Biscani
@bluescarni
Jun 24 2015 12:01 UTC
@isuruf the NDEBUG flag controls the compilation of assert() statements, I suggested Sumith to investigate if it would make sense to enable it for Sympy in release mode
Abinash Meher
@abinashmeher999
Jun 24 2015 12:01 UTC
@isuruf
  • For Manifest.txt, most of the gems I have seen, have one. We can't eliminate the possibility that they might be being used for documentation.
  • You are right, I will do that. Thanks for pointing it out. :smile:
  • I did not know that test files can also be added. I searched for it a little and found that they can be added by a different variable. like gem.test_files
  • I am very new to out of tree and in-tree builds. I might a little help with that.
  • Regarding moving lib/symengine/CMakeLists.txt to ext/symengine, will I need to change this line too?
Francesco Biscani
@bluescarni
Jun 24 2015 12:01 UTC
it's often used as a synonymous for "this is a release build"
Isuru Fernando
@isuruf
Jun 24 2015 12:02 UTC
@bluescarni, @Sumith1896. In symengine, we have the CMake variable SYMENGINE_ASSERT which uses asserts if turned on
Francesco Biscani
@bluescarni
Jun 24 2015 12:02 UTC
sympy -> symengine
Sumith Kulal
@Sumith1896
Jun 24 2015 12:02 UTC
Yes but our dependencies may not, that's why.
But my worry is the way we carry out tests
It is mostly asserts
Isuru Fernando
@isuruf
Jun 24 2015 12:03 UTC
Ah right.
Sumith Kulal
@Sumith1896
Jun 24 2015 12:04 UTC
Is our testing suite the correct way to do tests for c++ libraries?
Or can it be improved? I have no much idea how the other libraries carry out theirs.
Piranha uses Boost
Isuru Fernando
@isuruf
Jun 24 2015 12:06 UTC
You can use -DNDEBUG only for the symengine library.
Sumith Kulal
@Sumith1896
Jun 24 2015 12:07 UTC
You mean to say not compile tests
Isuru Fernando
@isuruf
Jun 24 2015 12:07 UTC
Nope. Tests are not part of the library. They are separate projects. So -DNDEBUG can be passed only to the library
Sumith Kulal
@Sumith1896
Jun 24 2015 12:08 UTC
This works for now. That's what I am doing.
Sumith Kulal
@Sumith1896
Jun 24 2015 12:21 UTC
But we should look into other libraries and see if ours is correct by standards.
@isuruf I now add the following two lines
set(CMAKE_CXX_FLAGS_RELEASE
    "${CMAKE_CXX_FLAGS_RELEASE} -DNDEBUG")
But this goes even for the tests, how do you do it only for the library?
Isuru Fernando
@isuruf
Jun 24 2015 12:26 UTC
Libraries I've seen uses their own assert macro. gmp has ASSERT. ginac has GINAC_ASSERT etc. MySQL has DBUG_ASSERT
I would recommend piranha change it's macro piranha_assert to use a define other than NDEBUG
Sumith Kulal
@Sumith1896
Jun 24 2015 12:32 UTC
I have seen a piranha_assert
But I don't think that's what we need
Isuru Fernando
@isuruf
Jun 24 2015 12:33 UTC
@abinashmeher999 Regarding moving lib/symengine/CMakeLists.txt to ext/symengine, you will need to change that line. Also, you will need to specify the install directory of the library symengine.so using set_target_properties
Francesco Biscani
@bluescarni
Jun 24 2015 12:34 UTC
piranha_assert is just a wrapper that adds a few extra info to a standard assert (like line number and file, IIRC)
Sumith Kulal
@Sumith1896
Jun 24 2015 12:35 UTC
Any way to switch it instead of NDEBUG?
Francesco Biscani
@bluescarni
Jun 24 2015 12:35 UTC
actually it does not... I confused it with piranha_throw... turns out it's just a standard assert
Sumith Kulal
@Sumith1896
Jun 24 2015 12:36 UTC
Since you use piranha_assert, how about a flag for that?
Francesco Biscani
@bluescarni
Jun 24 2015 12:36 UTC
I am not so convinced... my understanding is that a lot of packages use the NDEBUG macro
Sumith Kulal
@Sumith1896
Jun 24 2015 12:37 UTC
Okay
Francesco Biscani
@bluescarni
Jun 24 2015 12:37 UTC
I run gentoo linux so I see lots of compilation output from all sorts of packages :)
let's see what ondrej's opinion is
Sumith Kulal
@Sumith1896
Jun 24 2015 12:37 UTC
Let @certik give his inputs, we'll finalize then.
Francesco Biscani
@bluescarni
Jun 24 2015 12:37 UTC
if you include Piranha you will have Boost dependency as well, not sure how assertions are handled there
Isuru Fernando
@isuruf
Jun 24 2015 12:39 UTC
If there are many libraries using NDEBUG then I think we should add it to symengine if SYMENGINE_ASSERT is turned off.
Francesco Biscani
@bluescarni
Jun 24 2015 12:40 UTC
my opinion on this is that the standard assert should be used for detecting logical errors in a program, I am not keen on using it in unit testing
but I understand that if you are already using it extensively in the tests it might be difficult to change
Sumith Kulal
@Sumith1896
Jun 24 2015 12:41 UTC
If it is worry, I don't think it is a problem to change the tests now itself
It'll just get harder and harder as SymEngine expands
Isuru Fernando
@isuruf
Jun 24 2015 12:42 UTC
We wanted to use a testing framework instead of asserts, but nobody really looked into it. sympy/symengine#282
Sumith Kulal
@Sumith1896
Jun 24 2015 12:44 UTC
Cool, we should implement a testing framework.
Francesco Biscani
@bluescarni
Jun 24 2015 12:44 UTC
I think it's a reasonable thing to do... I am using boost unit testing because I am already depending on Boost for other things, but honestly I think any framework would be ok
there's Google's one too https://code.google.com/p/googletest/
Abinash Meher
@abinashmeher999
Jun 24 2015 12:45 UTC
Seems interesting! I have only heard of googletest. Will be nice to know a few other names.
@bluescarni @isuruf googletest uses assertions too. https://code.google.com/p/googletest/wiki/Primer#Assertions
Don't know if they are same as assert that we are currently using
Isuru Fernando
@isuruf
Jun 24 2015 12:49 UTC
We want to avoid too many hard dependencies. Catch is a simple header that we can include in the project tree itself.
Sumith Kulal
@Sumith1896
Jun 24 2015 12:49 UTC
Yes, Catch seems simple
Francesco Biscani
@bluescarni
Jun 24 2015 12:50 UTC
@abinashmeher999 they are called ASSERT and I am pretty confident they are not the same as the C ones
it's common in unit tests to use the name "assert" but it does not necessarily coincide with the C macro
Abinash Meher
@abinashmeher999
Jun 24 2015 12:51 UTC
I see. Yeah, same name doesn't imply they are same. Thanks!
Francesco Biscani
@bluescarni
Jun 24 2015 12:51 UTC
Sumith Kulal
@Sumith1896
Jun 24 2015 12:52 UTC
Yes, I have seen this in Pyranha
I will look into Catch
Abinash Meher
@abinashmeher999
Jun 24 2015 12:55 UTC
Catch has so much in a single header that it is difficult to believe that this is a testing framework.
Francesco Biscani
@bluescarni
Jun 24 2015 12:55 UTC
One of the advantages of using Boost, at least for a linux user, is that you can expect to find it packaged
so you don't need to ship it with your source tree or care about how it is built and things like this
but if catch is a header-only small library probably it's really easy to just include it in symengine
as a piece of history, for a while in Piranha I was convinced that having everything inside the same source tree (including pieces of boost) would be a good idea
but I have recanted that line of thought
Francesco Biscani
@bluescarni
Jun 24 2015 13:01 UTC
packaging is a solved problem from the engineering point of view since the nineties in linux distributions
but unfortunately we are stuck with non-free platforms (OSX, Windows) which force us to consider bundling everything together
because they have no standard package manager
ok rant ended, sorry about that :)
Abinash Meher
@abinashmeher999
Jun 24 2015 13:06 UTC
:smile: These are the times when we get to learn something.
Sumith Kulal
@Sumith1896
Jun 24 2015 13:12 UTC
Yes exactly :smile:
Catch can be added to SymEngine. Let me try it out.
Sumith Kulal
@Sumith1896
Jun 24 2015 13:45 UTC
The timings in report has been update, have a look: https://github.com/sympy/sympy/wiki/Benchmark-results-expand2b,-SymEngine
We are 1ms off Piranha but the bigger worry is the slowdown, I am looking into that
Ondřej Čertík
@certik
Jun 24 2015 14:52 UTC
@Sumith1896 see #282 for testing frameworks.
Sumith Kulal
@Sumith1896
Jun 24 2015 14:53 UTC
I looked into Catch
Certain blogposts have been really positive for Catch
Over Boost and Google, but I would like your opinion
Sumith Kulal
@Sumith1896
Jun 24 2015 14:59 UTC
Implementing Catch shouldn't take much time too.
Sumith Kulal
@Sumith1896
Jun 24 2015 15:05 UTC
@certik What are your views?
Ondřej Čertík
@certik
Jun 24 2015 15:09 UTC
@Sumith1896 yeah I like Catch a lot, I am watching https://www.youtube.com/watch?v=C2LcIp56i-8 now.
Let's use Catch. The advantage is that since it has just a few macros, if we later decide to use something else, we can easily define these macros ourselves. In other words, it looks like we only have to port our test suite once to Catch, if we switch again, it should be easy.
Sumith Kulal
@Sumith1896
Jun 24 2015 15:11 UTC
Cool, I managed some libs which use Catch
is one
Ondřej Čertík
@certik
Jun 24 2015 15:12 UTC
Cool.
Sumith Kulal
@Sumith1896
Jun 24 2015 15:13 UTC
I'll do this.
What are your views on NDEBUG compiler flags?
Ondřej Čertík
@certik
Jun 24 2015 15:13 UTC
Thanks!
Sumith Kulal
@Sumith1896
Jun 24 2015 15:14 UTC
We discussed but didn't get to conclusion.
Ondřej Čertík
@certik
Jun 24 2015 15:14 UTC
As to NDEBUG, I would make sure SymEngine doesn't use this flag, so that we are free to turn it on or off at will, without affecting tests. So by moving to Catch, that should do it. And we also have to modify our SYMENGINE_ASSERT to use something else than just an assert.
Then we can just set it properly for Piranha without side effects.
What other libraries use NDEBUG? I think these debugging flags should be implemented per library, so that you can, for example, use Piranha in optimized mode, but SymEngine in debug mode, etc.
Sumith Kulal
@Sumith1896
Jun 24 2015 15:26 UTC
So
  • SYMENGINE_ASSERT shouldn't use assert
  • How will cmake option of per library NDEBUG work? Is it possible?
Ondřej Čertík
@certik
Jun 24 2015 15:46 UTC
NDEBUG is not possible per library, so that's why we shouldn't use it in my opinion. Then we can easily switch SYMENGINE_ASSERT on and off, independent of NDEBUG. We'll use NDEBUG for Piranha or any other library that needs it.
Sumith Kulal
@Sumith1896
Jun 24 2015 17:03 UTC
I have a general query, do Shippable and Travis run the same tests?
Because Shippable fails too often.
@abinashmeher999 had pointed it out previously too, just got me wondering.
Ondřej Čertík
@certik
Jun 24 2015 17:03 UTC
They do, but Shippable has lots of internal bugs that prevent it to be as robust as Travis
Sumith Kulal
@Sumith1896
Jun 24 2015 17:04 UTC
So what purpose does Shippable serve?
Ondřej Čertík
@certik
Jun 24 2015 17:16 UTC
Travis sometimes takes a long time to start testing, because the whole sympy organization has the same quota. Shippable usually starts sooner. At least it used to, so we were able to quickly see if it passes tests. Lately it seems to be slower than Travis and more error prone. We can disable it, but for now just look at the Travis tests, if they pass, we are good to go.
Sumith Kulal
@Sumith1896
Jun 24 2015 17:17 UTC
Cool, Thanks. I was always wondering about it.
Sumith Kulal
@Sumith1896
Jun 24 2015 18:56 UTC
This message was deleted
@certik Where do you suggest I keep catch.hpp?