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

Typo in cp_pipe makeBrighterFatterKernel.py

    Details

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

      Description

      Two issues:

      (1) Function name in line 585 is mis-spelled.  Is spelled fitPtcAndNonlinearity and should be fitPtcAndNonLinearity.

      (2) Same function in line 585 is missing  the lookupTableArray input variable.

      I have verified that with these two fixes, it all runs OK.

      As an aside, I'm surprised there are not automated checks to check for function calls to functions that do not exist!

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Looks good; thank you.

            Show
            swinbank John Swinbank added a comment - Looks good; thank you.
            Hide
            plazas Andrés Alejandro Plazas Malagón added a comment - - edited

            Thank you, John. I have added the clarification that you suggest in the docstring of fitPtcAndNonLinearity. As for your second comment, I will include a couple of tests that check that the table is filled up as expected when the argument is provided, but I think I will do it in ticket DM-23681.

            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - - edited Thank you, John. I have added the clarification that you suggest in the docstring of fitPtcAndNonLinearity . As for your second comment, I will include a couple of tests that check that the table is filled up as expected when the argument is provided, but I think I will do it in ticket DM-23681 .
            Hide
            swinbank John Swinbank added a comment -

            Hey Andres — two things jump out at me —

            • Could you please update the docstring for fitPtcAndNonLinearity to indicate that tableArray is optional, and will be modified in-place if supplied?
            • Why remove the lookupTableArray argument from all of the tests? Wouldn't it be better to have at least one test that provides it, and checks that it is filled properly? (And, of course, to have at least one test that — as at present — doesn't provide it, and checks that the function still works.)
            Show
            swinbank John Swinbank added a comment - Hey Andres — two things jump out at me — Could you please update the docstring for fitPtcAndNonLinearity to indicate that tableArray is optional, and will be modified in-place if supplied? Why remove the lookupTableArray argument from all of the tests? Wouldn't it be better to have at least one test that provides it, and checks that it is filled properly? (And, of course, to have at least one test that — as at present — doesn't provide it, and checks that the function still works.)
            Hide
            plazas Andrés Alejandro Plazas Malagón added a comment -

            I have fixed the typo and made " lookupTableArray " optional in "ptc.py". This is an array that is modified in place and that is not needed in line 585 of "makeBrighterFatterKernel.py" (where "fitPtcAndNonLinearity" is used to get gains), as it is meant for constructing the linearizer (and this code is still under development). The unit tests for "fitPtcAndNonLinearity" are in "test_ptc.py", and they currently pass this table argument. I don't think DM-23681 would have made a difference.

            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - I have fixed the typo and made " lookupTableArray " optional in "ptc.py". This is an array that is modified in place and that is not needed in line 585 of "makeBrighterFatterKernel.py" (where "fitPtcAndNonLinearity" is used to get gains), as it is meant for constructing the linearizer (and this code is still under development). The unit tests for "fitPtcAndNonLinearity" are in "test_ptc.py", and they currently pass this table argument. I don't think DM-23681 would have made a difference.
            Hide
            plazas Andrés Alejandro Plazas Malagón added a comment - - edited

            I'll take a look. I also thought about DM-23681, which probably would have helped. I can look into that one first.

            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - - edited I'll take a look. I also thought about DM-23681 , which probably would have helped. I can look into that one first.
            Hide
            swinbank John Swinbank added a comment -

            Andrés Alejandro Plazas Malagón — this looks like an easy fix. Could you please take a look? Please also check what it would take to make a unit test to stop this happening in future (would DM-23681 have made a difference)?

            Show
            swinbank John Swinbank added a comment - Andrés Alejandro Plazas Malagón — this looks like an easy fix. Could you please take a look? Please also check what it would take to make a unit test to stop this happening in future (would DM-23681 have made a difference)?

              People

              • Assignee:
                plazas Andrés Alejandro Plazas Malagón
                Reporter:
                cslage Craig Lage
                Reviewers:
                John Swinbank
                Watchers:
                Andrés Alejandro Plazas Malagón, Craig Lage, John Swinbank
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Due:
                  Created:
                  Updated:
                  Resolved:

                  Summary Panel