Bug 251563

Summary:	Getting rid of the lapack dependancy using Eigen [patch]
Product:	[Applications] digikam	Reporter:	Thomas Capricelli <kdebugs.20.orzelf>
Component:	Portability-Runtime	Assignee:	Digikam Developers <digikam-bugs-null>
Status:	RESOLVED FIXED
Severity:	wishlist	CC:	bruno.schmidt, caulier.gilles, kevin.kofler, nucleo, rdieter, vivo75+kde
Priority:	NOR
Version:	1.5.0
Target Milestone:	---
Platform:	Gentoo Packages
OS:	Linux
Latest Commit:	http://commits.kde.org/digikam/84d745a59bd0f9d9697b50b9f8558d368d956faf	Version Fixed In:	3.2.0
Sentry Crash Report:
Attachments:	patch to remove dependancy on lapack updated patch Removing Clapack and using eigen3 instead Removing Clapack and using eigen3 instead updated patch to use Eigen3 instead clapack Remove Clapack and replace with Eigen3

Description Thomas Capricelli 2010-09-17 13:47:24 UTC

Created attachment 51754 [details]
patch to remove dependancy on lapack

Version:           unspecified (using Devel) 
OS:                Linux

Original mail : http://mail.kde.org/pipermail/digikam-devel/2010-September/045620.html

I was very surprised (shocked?) to read http://blog.akhuettel.de/2010/09/first-bump-digikam-140-kipi-plugins-130.html
Lot of people, and especially Gentoo people, think it's very bad to bundle copies of libraries (btw, I fully agree with them, but that's not the point here). Hence the gentoo package for digikam tries to use the systemwide lapack, which requires blas, and hence a fortran compiler.

Looking at the code, i could only see one single call to lapack : "dgesv_()" in libs/dimg/filters/sharp/matrix.cpp, which is a basic Ax=b solving (LU with pivoting). So one, and only one solving using a very common method makes digikam requires lapack/blas/fortran in gentoo, or, to stick with digikam point of view, lot of 3rdparty/ files in digikam source.

BUT, digikam already depends on KDE, which itself requires Eigen (http://eigen.tuxfamily.org), which provides such kind of linear algebra, and moreover in a very efficient manner, both code/API wise, and from a performance point of view.
Moreover, eigen was developed inside KDE to answer linear algebra needs among different KDE projects. Though it now lives its own way, there are still strong connections with KDE.

As you've probably guessed by now, I'm proposing a patch to do right this : use eigen instead of lapack to solve the linear system. Again, the dependancy on eigen is weak, as KDE already depends on it. I've used Eigen2 because eigen3 is not released yet (it's still in beta), and also because kde still depends on eigen2. I'll be  happy to update those few lines to eigen3 when required. In case you wonder, eigen works with a very broad range of compilers, oses, platforms, cpus... so it should in no way constrain digikam.

I've read http://www.digikam.org/drupal/contrib and the "HACKING" file and think/hope i comply with those. The webpage has great information such as "send patches this way", but no indication of where those patches should be sent. By the way, i have write access to kde svn, but I of course would not dare committing such a patch without asking you first. That can be useful later on though.

I've not tested this yet, mostly because i dont know how to test it, and because i first want to get your 'go' on this before spending more times on the patch. At least it compiles, and i think it does the same as before, there really are few lines to consider. The return value from the lapack function was not used (it could tell if the system has no solution), so i've kept it this way.

If you happen to agree on the idea, I could test it with someone with more in-depth knowledge of this code, and then commit it. I'd like to make some performance tests, too, just out of curiosity.

The source code in libs/dimg/filters/sharp/ is really weird by the way, it's supposed to be C++, but it's actually struct, with lot of c-like functions such as 'create matrix', 'free matrix', 'copy matrix', and such. It looks like gtk. My main purpose was to remove the lapack dependancy/code duplication, so i did a minmalist patch, but there's a lot of room for improvement here, by switching to actual C++, and maybe use slightly more of eigen (convolution and other stuff).


Reproducible: Didn't try

Comment 1 Thomas Capricelli 2010-09-17 17:41:14 UTC

Created attachment 51761 [details]
updated patch

Comment 2 caulier.gilles 2010-09-20 09:27:27 UTC

orzel,

Thanks for the patch...

I'm not fully agree to remove clapack code from digiKam core, because i want to limit external depencies for digiKam, especially under windows, where it's not easy to compile whole digiKam & co

But there is an advantage to use Eigen lib, if code use processors optimizations to speed-up code.

This is my proposal : set Eigen depency as optional. If it found, we compile digiKam core with it, else, internal Clapack code is used. I do the same with liblqr currently.

What do you think about ?

Gilles Caulier

Comment 3 Johannes Wienke 2010-09-20 11:09:21 UTC

Why not also check for lapack in the system and use this if available before falling back to the internal version?

Comment 4 Thomas Capricelli 2010-09-20 14:25:05 UTC

#2: as you want. Even on windows, the kde guys already have to deal with eigen anyway. Eigen is not a library, it's just a bunch of headers (template stuff). It is easier to integrate than libraries you have to find, link to, and so on.
#3: yes, why not.


The problem with all of this, is that you'll have three testcases to check, which means a lot more work for testing/releasing, and could mean more problems.

Comment 5 caulier.gilles 2010-10-22 10:39:49 UTC

Johaness, Orzel,

I apply patch from entry #254315 which check if Clapack library is avaialble to compile digiKam instead to use internal code.

Now it still to check Eigen shared library availability.

Orzel, please checkout svn trunk where the patch is applied and update your patch accordingly to play with Eigen library. I would apply it to 1.6.0 release...

Thanks in advance for your help

Gilles Caulier

Comment 6 Thomas Capricelli 2010-10-30 18:08:44 UTC

hi. i've updated svn and i'm working on it. I feel slightly uncomfortable, because i still dont know how to test it, and i guess nobody else has tested it. It feels weird to work on integration before tests...

Another point is that after this patch, according to configuration and to what is available at compile time, three very different code paths could be taken, which, from a maintainance point of view, is a nightmare.

Comment 7 Bruno Schmidt 2011-04-29 00:02:37 UTC

Why not remove clapack if Eigen are just headers.
Worst case you can do with it just like with clapack and bundle it but use the system one if it is available.

I am also a Gentoo user and will give it a try and post a patch when possible.

Comment 8 Thomas Capricelli 2011-04-29 00:22:47 UTC

Note than eigen3 is released now, and i expect other kde part will soon depend on it.

Comment 9 Marcel Wiesweg 2011-04-30 18:16:36 UTC

Eigen has several advantages, among them:
- It's 100% template C++, so compile-time only dependency, no runtime dependency (like our boost graph dependency)
- The compiler can fully optimize
- it's in a git repository and can be easily downloaded if not installed system wide

Comment 10 Thomas Capricelli 2011-05-01 01:05:04 UTC

Git is bad, we use Mercurial for eigen :-)

Comment 11 Kevin Kofler 2012-03-06 20:17:33 UTC

Nice, I think we'll want to use this in Fedora.

Comment 12 Thomas Capricelli 2012-03-06 22:30:25 UTC

I would too. If i can be of any help, just ask.

Comment 13 Francesco Riosa 2012-03-07 00:42:11 UTC

just an observation, follow a list of packages that depend on eigen in gentoo, all of them _require_ eigen-2 (including kde) so I think should be safe for a while to stick with this version of the template library.

>hom>viv>pro>dkD>src$ equery d dev-cpp/eigen
 * These packages depend on dev-cpp/eigen:
app-office/calligra-9999 (eigen ? dev-cpp/eigen:2)
kde-base/cantor-9999 (>=dev-cpp/eigen-2.0.3:2)
kde-base/kdeartwork-kscreensaver-9999 (eigen ? dev-cpp/eigen:2)
kde-base/kdeplasma-addons-9999 (dev-cpp/eigen:2)
kde-base/rocs-9999 (>=dev-cpp/eigen-2.0.3:2)
media-libs/opencv-2.3.1a-r1 (eigen ? dev-cpp/eigen:2)

Comment 14 caulier.gilles 2013-02-24 08:39:30 UTC

See my comment here :

https://bugs.kde.org/show_bug.cgi?id=295423#c6

Gilles Caulier

Comment 15 Gowtham Ashok 2013-03-24 13:50:32 UTC

Created attachment 78349 [details]
Removing Clapack and using eigen3 instead

This patch removes clapack and uses eigen3. Slightly modified from Giles' patch(eigen2->eigen3).

Comment 16 Gowtham Ashok 2013-03-24 18:14:14 UTC

Created attachment 78351 [details]
Removing Clapack and using eigen3 instead

Fixed bug in 78349: Faulty detection of Eigen3 library

Comment 17 Marcel Wiesweg 2013-03-24 18:24:42 UTC

Thanks for your work.
Did you send a compressed file? It is unreadable from the browser. You could also open a review request on KDE's review board.

Comment 18 Gowtham Ashok 2013-03-24 18:31:19 UTC

Yes.Its compressed as text is >65535 characters.Ok, will open review
request.

On Sun, Mar 24, 2013 at 11:54 PM, Marcel Wiesweg <marcel.wiesweg@gmx.de>wrote:

> https://bugs.kde.org/show_bug.cgi?id=251563
>
> --- Comment #17 from Marcel Wiesweg <marcel.wiesweg@gmx.de> ---
> Thanks for your work.
> Did you send a compressed file? It is unreadable from the browser. You
> could
> also open a review request on KDE's review board.
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.
> _______________________________________________
> Digikam-devel mailing list
> Digikam-devel@kde.org
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

Comment 19 caulier.gilles 2013-03-29 08:33:14 UTC

Created attachment 78470 [details]
updated patch to use Eigen3 instead clapack

I used your first patch posted in RB #109692. I encored these problems :

1/ Your previous patch do not work, to process image. This problem is not fixed in my version 3. Look my screenshot :

http://www.flickr.com/photos/digikam/8597933204/sizes/o/in/photostream/

Image Refocused is processed with a strange vibrato effect. Also i see that effect is not reproducible if you changes some parameters. Processing image is sometimes slow, and sometime very fast with same parameters...

Here i use Eigen3 version 3.05

2/ I review all cmake rules everywhere and patched code to use pre-processor conditional rules if Eigen3 is present or not. Please use this patch to investigate problem 1/

Gilles Caulier

Comment 20 Gowtham Ashok 2013-03-30 10:49:03 UTC

Created attachment 78503 [details]
Remove Clapack and replace with Eigen3

Fixed error in refocus filter(verified by comparing the matrices).Time taken to process image is similar(with reference to Clapack).
Added Eigen3 version in libsinfodlg.cpp, added one line in CMakeLists.txt to pass version number to it.

Comment 21 caulier.gilles 2013-03-30 17:22:40 UTC

Git commit 84d745a59bd0f9d9697b50b9f8558d368d956faf by Gilles Caulier.
Committed on 30/03/2013 at 18:19.
Pushed by cgilles into branch 'master'.

Remove clapack library from digiKam core used by Refocus tool.
Add new optional dependency to Eigen3 library to replace clapack
REVIEW: 109692
FIXED-IN: 3.2.0

M  +33   -52   CMakeLists.txt
M  +1    -0    NEWS
M  +3    -3    README
D  +0    -44   cmake/modules/FindCLAPACK.cmake
A  +81   -0    cmake/modules/FindEigen3.cmake
M  +1    -11   digikam/CMakeLists.txt
M  +3    -0    digikam/utils/config-digikam.h.cmake
M  +11   -1    imageplugins/enhance/sharpentool.cpp
D  +0    -12   libs/3rdparty/clapack/LICENCE
D  +0    -2    libs/3rdparty/clapack/README
D  +0    -16   libs/3rdparty/clapack/abort_.c
D  +0    -158  libs/3rdparty/clapack/blaswrap.h
D  +0    -5079 libs/3rdparty/clapack/clapack.h
D  +0    -94   libs/3rdparty/clapack/close.c
D  +0    -313  libs/3rdparty/clapack/dgemm.c
D  +0    -143  libs/3rdparty/clapack/dger.c
D  +0    -117  libs/3rdparty/clapack/dgesv.c
D  +0    -157  libs/3rdparty/clapack/dgetf2.c
D  +0    -197  libs/3rdparty/clapack/dgetrf.c
D  +0    -159  libs/3rdparty/clapack/dgetrs.c
D  +0    -143  libs/3rdparty/clapack/dlaswp.c
D  +0    -62   libs/3rdparty/clapack/dscal.c
D  +0    -81   libs/3rdparty/clapack/dswap.c
D  +0    -404  libs/3rdparty/clapack/dtrsm.c
D  +0    -121  libs/3rdparty/clapack/endfile.c
D  +0    -271  libs/3rdparty/clapack/err.c
D  +0    -226  libs/3rdparty/clapack/f2c.h
D  +0    -109  libs/3rdparty/clapack/fio.h
D  +0    -516  libs/3rdparty/clapack/fmt.c
D  +0    -100  libs/3rdparty/clapack/fmt.h
D  +0    -45   libs/3rdparty/clapack/fmtlib.c
D  +0    -28   libs/3rdparty/clapack/fp.h
D  +0    -61   libs/3rdparty/clapack/idamax.c
D  +0    -150  libs/3rdparty/clapack/ieeeck.c
D  +0    -610  libs/3rdparty/clapack/ilaenv.c
D  +0    -101  libs/3rdparty/clapack/lsame.c
D  +0    -291  libs/3rdparty/clapack/open.c
D  +0    -44   libs/3rdparty/clapack/s_cmp.c
D  +0    -51   libs/3rdparty/clapack/s_copy.c
D  +0    -42   libs/3rdparty/clapack/s_stop.c
D  +0    -31   libs/3rdparty/clapack/sfe.c
D  +0    -45   libs/3rdparty/clapack/sig_die.c
D  +0    -53   libs/3rdparty/clapack/util.c
D  +0    -276  libs/3rdparty/clapack/wref.c
D  +0    -365  libs/3rdparty/clapack/wrtfmt.c
D  +0    -73   libs/3rdparty/clapack/wsfe.c
D  +0    -58   libs/3rdparty/clapack/xerbla.c
M  +4    -6    libs/dialogs/libsinfodlg.cpp
M  +6    -1    libs/dimg/filters/dimgfiltermanager.cpp
M  +19   -21   libs/dimg/filters/sharp/matrix.cpp
M  +0    -2    libs/dimg/filters/sharp/matrix.h
M  +41   -10   libs/dimg/filters/sharp/sharpsettings.cpp
M  +1    -1    libs/dimg/filters/sharp/sharpsettings.h
M  +31   -19   utilities/queuemanager/basetools/enhance/sharpen.cpp

http://commits.kde.org/digikam/84d745a59bd0f9d9697b50b9f8558d368d956faf