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?
@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.
@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.rbfiles are not added to the gem. - Add support for out of tree builds
- Move
lib/symengine/CMakeLists.txttoext/symengine/
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.
Also I'll have to look into the slowdown of benchmarks I had previously reported.
@certik you can run the
expand2d in sympy/symengine#470
to try out the benchmark in your machine
@Sumith1896, why do you need
-DNDEBUG? Can you post the full output of cmake command
@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
@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
@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
@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.txttoext/symengine, will I need to change this line too?
it's often used as a synonymous for "this is a release build"
@bluescarni, @Sumith1896. In
symengine, we have the CMake variable SYMENGINE_ASSERT which uses asserts if turned on
sympy -> symengine
Yes but our dependencies may not, that's why.
But my worry is the way we carry out tests
It is mostly
asserts
Ah right.
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
You can use
-DNDEBUG only for the symengine library.
You mean to say not compile tests
Nope. Tests are not part of the library. They are separate projects. So
-DNDEBUG can be passed only to the library
This works for now. That's what I am doing.
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?
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
I have seen a
piranha_assert
But I don't think that's what we need
@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
piranha_assert is just a wrapper that adds a few extra info to a standard assert (like line number and file, IIRC)
Any way to switch it instead of
NDEBUG?
actually it does not... I confused it with
piranha_throw... turns out it's just a standard assert
Since you use
piranha_assert, how about a flag for that?
I am not so convinced... my understanding is that a lot of packages use the
NDEBUG macro
Okay
I run gentoo linux so I see lots of compilation output from all sorts of packages :)
let's see what ondrej's opinion is
Let @certik give his inputs, we'll finalize then.
if you include Piranha you will have Boost dependency as well, not sure how assertions are handled there
If there are many libraries using
NDEBUG then I think we should add it to symengine if SYMENGINE_ASSERT is turned off.
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
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
We wanted to use a testing framework instead of
asserts, but nobody really looked into it. sympy/symengine#282
Cool, we should implement a testing framework.
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/
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
We want to avoid too many hard dependencies.
Catch is a simple header that we can include in the project tree itself.
Yes,
Catch seems simple
@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
I see. Yeah, same name doesn't imply they are same. Thanks!
also Python does a similar thing: https://docs.python.org/2/library/unittest.html#assert-methods
vs the
assert statement in the language: https://docs.python.org/2/reference/simple_stmts.html#grammar-token-assert_stmt
Yes, I have seen this in
Pyranha
I will look into
Catch
Catch has so much in a single header that it is difficult to believe that this is a testing framework.
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
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 :)
:smile: These are the times when we get to learn something.
Yes exactly :smile:
Catch can be added to SymEngine. Let me try it out.
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
@Sumith1896 see #282 for testing frameworks.
I looked into
Catch
Certain blogposts have been really positive for
Catch
Over
Boost and Google, but I would like your opinion
Implementing
Catch shouldn't take much time too.
@certik What are your views?
@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.
Cool, I managed some libs which use Catch
is one
Cool.
I'll do this.
What are your views on
NDEBUG compiler flags?
Thanks!
We discussed but didn't get to conclusion.
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.
So
- SYMENGINE_ASSERT shouldn't use
assert - How will cmake option of per library NDEBUG work? Is it possible?
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.
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.
They do, but Shippable has lots of internal bugs that prevent it to be as robust as Travis
So what purpose does Shippable serve?
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.
Cool, Thanks. I was always wondering about it.
This message was deleted
@certik Where do you suggest I keep
catch.hpp?