Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-18895

Use std::ptrdiff_t as index type in jointcal Eigen objects

    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

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            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?

            Show
            Parejkoj John Parejko added a comment - 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?
            Hide
            price 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?

            Show
            price 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?
            Hide
            tjenness Tim Jenness added a comment - - edited

            I see that newer versions of Eigen have some fixes for int to Index but no idea if that's relevant.

            Show
            tjenness Tim Jenness added a comment - - edited I see that newer versions of Eigen have some fixes for int to Index but no idea if that's relevant.
            Hide
            jbosch Jim Bosch added a comment - - edited

            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.

            Show
            jbosch Jim Bosch added a comment - - edited 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.
            Hide
            price 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.

            Show
            price 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.
            Hide
            tjenness Tim Jenness added a comment -

            Can we update eigen to 3.3.7 whilst we are at it? No reason to live with the older version.

            Show
            tjenness Tim Jenness added a comment - Can we update eigen to 3.3.7 whilst we are at it? No reason to live with the older version.
            Hide
            price 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.

            Show
            price 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.
            Hide
            price 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.

            Show
            price 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.
            Hide
            Parejkoj John Parejko added a comment - - edited

            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)?

            Show
            Parejkoj John Parejko added a comment - - edited 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)?
            Hide
            tjenness Tim Jenness added a comment -

            No RFC is required for a patch release of eigen.

            Show
            tjenness Tim Jenness added a comment - No RFC is required for a patch release of eigen.
            Hide
            price Paul Price added a comment -

            It's on this ticket; there's a PR.

            Show
            price Paul Price added a comment - It's on this ticket; there's a PR.
            Hide
            Parejkoj John Parejko added a comment -

            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

            Show
            Parejkoj John Parejko added a comment - 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
            Hide
            tjenness Tim Jenness added a comment -

            No PR and the branch wasn't renamed I think: https://github.com/lsst/eigen/tree/u/price/debug-20190408

            Show
            tjenness Tim Jenness added a comment - No PR and the branch wasn't renamed I think: https://github.com/lsst/eigen/tree/u/price/debug-20190408
            Hide
            price 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

            Show
            price 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
            Hide
            Parejkoj John Parejko added a comment -

            Is the plan to move these branches to this ticket name, or merge them as-is?

            Show
            Parejkoj John Parejko added a comment - Is the plan to move these branches to this ticket name, or merge them as-is?
            Hide
            price 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.

            Show
            price 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.
            Hide
            Parejkoj John Parejko added a comment -

            See the comments on the jointcal PR. Should I close my PR and move my tickets/DM-18895 branch to u/parejkoj/DM-18895 so you can move your debug branch over?

            Show
            Parejkoj John Parejko added a comment - See the comments on the jointcal PR. Should I close my PR and move my tickets/ DM-18895 branch to u/parejkoj/ DM-18895 so you can move your debug branch over?
            Hide
            Parejkoj John Parejko added a comment -

            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").

            Show
            Parejkoj John Parejko added a comment - 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").
            Hide
            Parejkoj John Parejko added a comment -

            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.

            Show
            Parejkoj John Parejko added a comment - 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.
            Hide
            price Paul Price added a comment -

            I've put this on the official ticket branches and created new GitHub PRs.

            Show
            price Paul Price added a comment - I've put this on the official ticket branches and created new GitHub PRs.
            Hide
            price Paul Price added a comment -

            Jenkins is green.

            Show
            price Paul Price added a comment - Jenkins is green.
            Hide
            price Paul Price added a comment -

            Merged to master.

            Show
            price Paul Price added a comment - Merged to master.

              People

              • Assignee:
                price Paul Price
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                John Parejko
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Paul Price, Tim Jenness, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel