I've submitted a pull request on GitHub for some general clean-up of the PaStiX support module. Do you accept pull requests there or would you rather have them in one place (i.e. here)? This worked is aimed at making sure the wrapper is a little more robust and provides a better starting point for the upcoming pull request for the MUMPS wrapper (mostly complete).
Please don't make PRs on the github mirror. Either attach a patch here, or make a PR to the original bitbucket mercurial.
Thanks for the reply Christoph. Then with the magic of copy and paste: Questions: Do you have a clang-format to use? The formatting is terribly inconsistent. Is this done on merge instead? Why are the includes for pastix not done inside the .h file? My linter throws up all sorts of errors about missing types. More work is required to use pastix_int_t as it current assumes sizeof(int) == sizeof(pastix_int_t). Happy for me to pop that in too? On the side, a new pastix has released that's about 10000000x easier to build and the interface is much simpler. Thoughts on including this instead and killing off this older interface?
Created attachment 817 [details] Spaces where no spaces have been before.
Created attachment 818 [details] Inlines aren't welcome.
Created attachment 819 [details] IPARM: You don't need to to twice!!
Created attachment 820 [details] Leveraging the future. Views views views!
Created attachment 821 [details] Only have support for the PaStiX sequential library. So just look for that Thoughts? Not sure why it was the other way around before.
Created attachment 822 [details] Package case change in CMake This is puzzling. Perhaps this is recently enforced on CMake? I have 3.10 on my system. Ideas why?
Created attachment 823 [details] Consistency changes I changed some of the loop counters from C style to c++ and used the index type to avoid some overflow problems. My previous comments are related to this as it's currently a sloppy job (as PaStiX needs to check its int type against Eigen's). Also some parenthesis were removed (operator precedence should take care of this anyhow). Made the IPARM calls consistent with [] vs ().
Created attachment 824 [details] Documentation improvement & formatting
(In reply to Darcy from comment #2) > Do you have a clang-format to use? The formatting is terribly > inconsistent. Is this done on merge instead? We never had very strict formatting rules: http://eigen.tuxfamily.org/index.php?title=Developer%27s_Corner#Eigen_hacking I'm personally for reducing pure white-space changes. (I'd only clean up white space where I change code anyway) > Why are the includes for pastix not done inside the .h file? My linter > throws up all sorts of errors about missing types. Related: http://eigen.tuxfamily.org/bz/show_bug.cgi?id=339 I would not object to adding includes to individual .h files. I think there also was a mailing list discussion about that long time ago. I don't remember exactly if we agreed on something. > More work is required to use pastix_int_t as it current assumes > sizeof(int) == sizeof(pastix_int_t). Happy for me to pop that in too? I don't really know PaStiX. Does this essentially require that PaStiX only works if the InnerIndex type of the sparse matrix is compatible? > On the side, a new pastix has released that's about 10000000x easier to > build and the interface is much simpler. Thoughts on including this instead > and killing off this older interface? I've never used that either. Maybe it is indeed simpler to add a new module for that (under different name) and eventually deprecate this module.
(In reply to Darcy from comment #9) > Created attachment 823 [details] > Consistency changes > > I changed some of the loop counters from C style to c++ and used the index > type to avoid some overflow problems. My previous comments are related to > this as it's currently a sloppy job (as PaStiX needs to check its int type > against Eigen's). +1 for C++ loop counters and using Index where possible. > Also some parenthesis were removed (operator precedence should take care of > this anyhow). Those might have been there to avoid compiler warnings or to improve readability. (Not everybody knows precedence rules by heart ...) > Made the IPARM calls consistent with [] vs (). I'd prefer the [] here to make it more obvious that these are array accesses and not function calls. It seems the inconsistent ()-accesses where added later than the [].
(In reply to Christoph Hertzberg from comment #11) > (In reply to Darcy from comment #2) > > Do you have a clang-format to use? The formatting is terribly > > inconsistent. Is this done on merge instead? > > We never had very strict formatting rules: > http://eigen.tuxfamily.org/index.php?title=Developer%27s_Corner#Eigen_hacking > I'm personally for reducing pure white-space changes. (I'd only clean up > white space where I change code anyway) If there was some provided clang-format it would be automated and this wouldn't be a discussion. If someone doesn't like the project clang-format, they can apply their own when developing and then it's changed to the 'official' style on commit. > > > Why are the includes for pastix not done inside the .h file? My linter > > throws up all sorts of errors about missing types. > > Related: > http://eigen.tuxfamily.org/bz/show_bug.cgi?id=339 > I would not object to adding includes to individual .h files. I think there > also was a mailing list discussion about that long time ago. I don't > remember exactly if we agreed on something. > > > More work is required to use pastix_int_t as it current assumes > > sizeof(int) == sizeof(pastix_int_t). Happy for me to pop that in too? > > I don't really know PaStiX. Does this essentially require that PaStiX only > works if the InnerIndex type of the sparse matrix is compatible? Not exactly sure. I'd rather static_assert this and just make sure pastix_int_t == Index. I'd rather not see a raw int in a wrapper when this is handled by both APIs. > > > On the side, a new pastix has released that's about 10000000x easier to > > build and the interface is much simpler. Thoughts on including this instead > > and killing off this older interface? > > I've never used that either. Maybe it is indeed simpler to add a new module > for that (under different name) and eventually deprecate this module. I agree. I'm bad at naming modules. PaStiX6Support?
(In reply to Christoph Hertzberg from comment #12) > (In reply to Darcy from comment #9) > > Created attachment 823 [details] > > Consistency changes > > > > I changed some of the loop counters from C style to c++ and used the index > > type to avoid some overflow problems. My previous comments are related to > > this as it's currently a sloppy job (as PaStiX needs to check its int type > > against Eigen's). > > +1 for C++ loop counters and using Index where possible. > > > Also some parenthesis were removed (operator precedence should take care of > > this anyhow). > > Those might have been there to avoid compiler warnings or to improve > readability. (Not everybody knows precedence rules by heart ...) :O They don't!? I might go a little further then and not rely on everyone knowing implicit integer boolean conversion and say if (index != 0 or !=1) then? No room for error with that. > > Made the IPARM calls consistent with [] vs (). > > I'd prefer the [] here to make it more obvious that these are array accesses > and not function calls. It seems the inconsistent ()-accesses where added > later than the []. Sure. I'll change them all to []
Created attachment 825 [details] Unused function? I couldn't see where this function was ever used.
Thank you for all the cleanup. Regarding Pastix6, I see two options: 1) if the main pastix header is the same and that the API of Eigen::Pastix* will be exactly the same, then we can branch using #if directives depending on which version is included. 2) make a new PaStiX6Support module with Pastix6L* classes. The second option might be saner and more future proof.
Created attachment 828 [details] Consistency changes Do I need to provide you the rebased commits based on this commit? It would invalid 824 and 825.
(In reply to Gael Guennebaud from comment #16) > Thank you for all the cleanup. Thanks to you and Christoph for reviewing. > > Regarding Pastix6, I see two options: > > 1) if the main pastix header is the same and that the API of Eigen::Pastix* > will be exactly the same, then we can branch using #if directives depending > on which version is included. > > 2) make a new PaStiX6Support module with Pastix6L* classes. > > The second option might be saner and more future proof. There's a change in header file name which makes the preprocessor method more difficult than #2. I'll introduce a PaStiX6Support module with new classes. There's some void* casting goodness in their new API for poor man polymorphism so it should be easier to wrap.
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1507.