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 -

            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: