Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: jointcal
-
Labels:None
-
Story Points:1
-
Sprint:AP S19-5, AP S19-6
-
Team:External
Description
Paul Price noticed that the use of unsigned (== uint32, usually) as the index type for Eigen matrices in jointcal might be a problem:
- Eigen docs appear to want a signed integer;
- our number of sources in some of our processing is approaching 2^31.
There's no hard evidence that this is what's causing the segfault we're seeing when processing the (larger, still-proprietary) HSC UltraDeep, and since unsigned should be able to handle 2^32, I'm personally skeptical that this is the problem. But it should be easy to fix, and a good thing to do regardless, especially for the first reason.
std::ptrdiff_t is probably the right choice to use instead - that will be a signed integer as large as the pointer type (i.e. int64, usually), and hence the largest unsigned type that's actually usable as an index.
Attachments
Issue Links
- blocks
-
- To Do
Activity
| Field | Original Value | New Value |
|---|---|---|
| Status | To Do [ 10001 ] | In Progress [ 3 ] |
| Watchers | Jim Bosch, John Swinbank [ Jim Bosch, John Swinbank ] | Jim Bosch, John Swinbank, Paul Price, Yusra AlSayyad [ Jim Bosch, John Swinbank, Paul Price, Yusra AlSayyad ] |
| Reviewers | Paul Price [ price ] | |
| Status | In Progress [ 3 ] | In Review [ 10004 ] |
| Sprint | AP S19-5 [ 833 ] | |
| Priority | Undefined [ 10000 ] | Major [ 3 ] |
I've generated pull requests for my proposed changes in jointcal and jointcal_cholmod. I have left these on my u branch for the time being, since I don't want to clobber the existing ticket branch before there is general agreement. As I've noted on the jointcal PR:
it does introduce extensive changes to the index types (probably more than strictly required) and a dirty hack to get around Eigen's lack of support for these larger index types
While I think these are reasonable changes (even if the second is undesirable, I believe it is necessary with the current state of Eigen), perhaps someone has an alternative to consider?
Paul Price
added a comment - I've generated pull requests for my proposed changes in jointcal and jointcal_cholmod . I have left these on my u branch for the time being, since I don't want to clobber the existing ticket branch before there is general agreement. As I've noted on the jointcal PR:
it does introduce extensive changes to the index types (probably more than strictly required) and a dirty hack to get around Eigen's lack of support for these larger index types
While I think these are reasonable changes (even if the second is undesirable, I believe it is necessary with the current state of Eigen), perhaps someone has an alternative to consider? I see that newer versions of Eigen have some fixes for int to Index but no idea if that's relevant.
| Attachment | cholmod-longs.patch [ 37848 ] |
There is an unreleased change on Eigen master that seems quite relevant; most (but not all) of it is in this commit: https://github.com/eigenteam/eigen-git-mirror/commit/9cb9646c471878cd6843bcf5a272410121360b9c
Since it's unreleased, perhaps we should try to add a patch to our Eigen build that's just the diff between Eigen master and the last release for this file (might as well upgrade from 3.3.4 to 3.3.7 at the same time, though that won't affect this file)? I've attached such a patch (cholmod-longs.patch
) to the ticket, though it's created with the default git diff options and I don't know if it's better to make patches some other way.
Glancing at the commit history for the file (https://github.com/eigenteam/eigen-git-mirror/commits/master/Eigen/src/CholmodSupport/CholmodSupport.h; note that 3.3 was branched around Nov. 2016), I don't see anything that looks tied to other changes on Eigen master.
That's awesome, Jim, thanks!
The patch applies cleanly. I've dumped it into the Eigen build, removed my hack in jointcal, and started a Jenkins run to see if we can get out of jail free.
Paul Price
added a comment - That's awesome, Jim, thanks!
The patch applies cleanly. I've dumped it into the Eigen build, removed my hack in jointcal, and started a Jenkins run to see if we can get out of jail free. Can we update eigen to 3.3.7 whilst we are at it? No reason to live with the older version.
Updated to eigen 3.3.7 and did a full Jenkins run through ci_hsc. I'm now going to ensure we get through that segfault with these changes, but I think that we're good to go. John Parejko, I would appreciate your comments on the proposed changes before I clobber your work on the ticket branch.
Paul Price
added a comment - Updated to eigen 3.3.7 and did a full Jenkins run through ci_hsc. I'm now going to ensure we get through that segfault with these changes, but I think that we're good to go. John Parejko , I would appreciate your comments on the proposed changes before I clobber your work on the ticket branch. Verified that these changes get us past the segfault. I'll clean up the changes a bit in preparation for John Parejko's review this afternoon.
Paul Price
added a comment - Verified that these changes get us past the segfault. I'll clean up the changes a bit in preparation for John Parejko 's review this afternoon. | Reviewers | Paul Price [ price ] | John Parejko [ parejkoj ] |
| Assignee | John Parejko [ parejkoj ] | Paul Price [ price ] |
Is the eigen update on this ticket, or a different one? Do we need to RFC that (I suspect not, since everyone that might care has already asked for it here)?
No RFC is required for a patch release of eigen.
It's on this ticket; there's a PR.
Paul Price
added a comment - It's on this ticket; there's a PR. Can you please link to the PR? Jira isn't picking it up and I don't see anything at https://github.com/lsst/eigen
No PR and the branch wasn't renamed I think: https://github.com/lsst/eigen/tree/u/price/debug-20190408
Sorry, I was wrong (saw 3 PRs in Jira, and assumed this was one of them). I have now made a PR: https://github.com/lsst/eigen/pull/6
Paul Price
added a comment - Sorry, I was wrong (saw 3 PRs in Jira, and assumed this was one of them). I have now made a PR: https://github.com/lsst/eigen/pull/6 Is the plan to move these branches to this ticket name, or merge them as-is?
I was planning to put the work currently on the official ticket branches to a supplementary branch (perhaps a u/parejko branch?) and then put what's on u/price/debug-20190408 onto the official ticket branches before merging.
Paul Price
added a comment - I was planning to put the work currently on the official ticket branches to a supplementary branch (perhaps a u/parejko branch?) and then put what's on u/price/debug-20190408 onto the official ticket branches before merging. Oh, and please make the new PRs (from your moved branches to master) match the dev guide so the goal is more obvious ("DM-NNNN: JIRA Ticket Title").
With the changes on the jointcal branch, this looks good. I think you're going to have to make new PRs once you've renamed the three debug branches, though.
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
I've put this on the official ticket branches and created new GitHub PRs.
Paul Price
added a comment - I've put this on the official ticket branches and created new GitHub PRs. Jenkins is green.
Paul Price
added a comment - Jenkins is green. | Sprint | AP S19-5 [ 833 ] | AP S19-5, AP S19-6 [ 833, 834 ] |
| Team | Alert Production [ 10300 ] | External [ 12117 ] |
Merged to master.
Paul Price
added a comment - Merged to master. Paul Price
made changes -
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
Paul Price: I've gotten this to build locally without warnings (Ubuntu 18.04), and I think I caught all of the necessary places where it might matter what the matrix size was. Can you please both review the PR, and see if it resolves the segfault?