New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 1507 - PaStiX support maintenance
Summary: PaStiX support maintenance
Status: NEW
Alias: None
Product: Eigen
Classification: Unclassified
Component: General (show other bugs)
Version: 3.4 (development)
Hardware: All All
: Normal Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-21 15:30 UTC by Darcy
Modified: 2018-02-10 21:38 UTC (History)
4 users (show)



Attachments
Spaces where no spaces have been before. (23.00 KB, patch)
2018-02-08 16:55 UTC, Darcy
no flags Details | Diff
Inlines aren't welcome. (762 bytes, patch)
2018-02-08 16:56 UTC, Darcy
no flags Details | Diff
IPARM: You don't need to to twice!! (707 bytes, patch)
2018-02-08 16:57 UTC, Darcy
no flags Details | Diff
Leveraging the future. Views views views! (920 bytes, patch)
2018-02-08 16:58 UTC, Darcy
no flags Details | Diff
Only have support for the PaStiX sequential library. So just look for that (626 bytes, patch)
2018-02-08 17:00 UTC, Darcy
no flags Details | Diff
Package case change in CMake (561 bytes, patch)
2018-02-08 17:02 UTC, Darcy
no flags Details | Diff
Consistency changes (3.68 KB, patch)
2018-02-08 17:07 UTC, Darcy
no flags Details | Diff
Documentation improvement & formatting (5.23 KB, patch)
2018-02-08 17:11 UTC, Darcy
no flags Details | Diff
Unused function? (903 bytes, patch)
2018-02-08 18:13 UTC, Darcy
no flags Details | Diff
Consistency changes (8.28 KB, patch)
2018-02-10 21:31 UTC, Darcy
no flags Details | Diff

Description Darcy 2018-01-21 15:30:34 UTC
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).
Comment 1 Christoph Hertzberg 2018-02-08 14:54:01 UTC
Please don't make PRs on the github mirror. Either attach a patch here, or make a PR to the original bitbucket mercurial.
Comment 2 Darcy 2018-02-08 16:38:58 UTC
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?
Comment 3 Darcy 2018-02-08 16:55:06 UTC
Created attachment 817 [details]
Spaces where no spaces have been before.
Comment 4 Darcy 2018-02-08 16:56:40 UTC
Created attachment 818 [details]
Inlines aren't welcome.
Comment 5 Darcy 2018-02-08 16:57:48 UTC
Created attachment 819 [details]
IPARM: You don't need to to twice!!
Comment 6 Darcy 2018-02-08 16:58:56 UTC
Created attachment 820 [details]
Leveraging the future.  Views views views!
Comment 7 Darcy 2018-02-08 17:00:49 UTC
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.
Comment 8 Darcy 2018-02-08 17:02:14 UTC
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?
Comment 9 Darcy 2018-02-08 17:07:33 UTC
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 ().
Comment 10 Darcy 2018-02-08 17:11:50 UTC
Created attachment 824 [details]
Documentation improvement & formatting
Comment 11 Christoph Hertzberg 2018-02-08 17:33:07 UTC
(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.
Comment 12 Christoph Hertzberg 2018-02-08 17:43:47 UTC
(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 [].
Comment 13 Darcy 2018-02-08 17:49:56 UTC
(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?
Comment 14 Darcy 2018-02-08 17:55:03 UTC
(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 []
Comment 15 Darcy 2018-02-08 18:13:44 UTC
Created attachment 825 [details]
Unused function?

I couldn't see where this function was ever used.
Comment 16 Gael Guennebaud 2018-02-08 20:06:42 UTC
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.
Comment 17 Darcy 2018-02-10 21:31:39 UTC
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.
Comment 18 Darcy 2018-02-10 21:38:02 UTC
(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.

Note You need to log in before you can comment on or make changes to this bug.