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

Fix to DM-2883 isn't quite right

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      The fix to DM-2883 (remove illegal PVi_j cards) isn't quite right, and the error was masked by a piece of code elsewhere that duplicated the functionality.

      The issues is that while PV1_[1-4] cards are indeed valid, the ones that SCAMP writes are not. So we should remove them too, if there are any other SCAMP TPV coefficients.

      The masking code was a unilateral removal of PVi_j cards dating back years.

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            $ git diff --stat origin/master..
             src/image/Wcs.cc     | 15 ---------------
             src/image/makeWcs.cc | 12 +++++++++---
             2 files changed, 9 insertions(+), 18 deletions(-)
            

            Show
            rhl Robert Lupton added a comment - $ git diff --stat origin/master.. src/image/Wcs.cc | 15 --------------- src/image/makeWcs.cc | 12 +++++++++--- 2 files changed, 9 insertions(+), 18 deletions(-)
            Hide
            swinbank John Swinbank added a comment -

            The code as written won't compile on GCC 4.4, which doesn't support range-based for. That's an easy fix, though. I made a couple of other minor comments in the GitHub PR.

            The previous code renamed, rather than simply removed, the troublesome header cards. I'm not sure if that was useful. Do you think it's behaviour that's worth preserving?

            How about a test case?

            Show
            swinbank John Swinbank added a comment - The code as written won't compile on GCC 4.4, which doesn't support range-based for. That's an easy fix, though. I made a couple of other minor comments in the GitHub PR. The previous code renamed, rather than simply removed, the troublesome header cards. I'm not sure if that was useful. Do you think it's behaviour that's worth preserving? How about a test case?
            Hide
            rhl Robert Lupton added a comment -

            Ran buildbot (successfully), merged, and pushed to master

            Show
            rhl Robert Lupton added a comment - Ran buildbot (successfully), merged, and pushed to master

              People

              • Assignee:
                rhl Robert Lupton
                Reporter:
                rhl Robert Lupton
                Reviewers:
                John Swinbank
                Watchers:
                John Swinbank, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel