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

Add a unit test for aperture corrections in measurement task

    Details

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

      Description

      DM-436 adds code to meas_base that allows one to run a subset of measurement algorithms based on execution order. This addition should have a unit test.

      DM-436 also tasks to measure and apply aperture correction. Those tasks should have unit tests.

        Attachments

          Issue Links

            Activity

            Hide
            sullivan Ian Sullivan added a comment -

            If you have time, please review these two unit tests I've added for measuring and applying aperture correction. I realize that the commit history will need to be cleaned up before this can be merged into master, but I would appreciate suggestions on best practices (e.g. one commit per file, or one for the tests and another for the minor changes to the functions being tested).

            Show
            sullivan Ian Sullivan added a comment - If you have time, please review these two unit tests I've added for measuring and applying aperture correction. I realize that the commit history will need to be cleaned up before this can be merged into master, but I would appreciate suggestions on best practices (e.g. one commit per file, or one for the tests and another for the minor changes to the functions being tested).
            Hide
            rowen Russell Owen added a comment - - edited

            Thank you very much for writing such thorough tests. Overall this looks good.

            My main suggestion is to try to factor out common code (e.g. in to separate methods or functions). Benefits include:

            • If you find a bug you only have to fix it in one place
            • It usually makes your intent clearer by highlight important differences.

            Please make sure every method (and class and free function) has a doc string explaining what it is doing. Most of your test methods are missing this.

            I am nervous about using assertEqual to compare numeric values that should be the same. Unless you have a very strong reason to expect floats to be identical I recommend that you use assertAlmostEqual instead, with a suitable tolerance.

            Also, this is picky, but please provide a space after the leading "#" in Python comments. It makes them easier to read.

            A few specific bits:

            testMeasureApCorr.py

            testApCorrMapKeys: I don't think it is safe to assume an order for the keys (though I may be wrong). If order is not assured then this is safer: self.assertEqual(set(struct.apCorrMap.keys()), set(key_names))

            testTooFewSources: I don't understand what this test is doing (though a method doc string would probably explain). And it's testing zero sources, which is a stretch for the term "too few".

            testSourceNotUsed: I think this test would be more believable if the source you add had data associated with it (including a centroid that is presumably outside the bbox).

            testApertureMeasOnes and testApertureMeasTens are one instance of similar code that could be factored out into common code.

            Please run pyflakes and fix the warnings, including an imported package that is not used and setting variables that are not used. In some cases a line can be deleted; in others perhaps you want to call a function to be sure it doesn't raise an exception, in which case don't assign the returned result to a variable.

            testApplyApCorr.py

            testFailureFlagged, testSuccessUnflagged, testCatFluxUnchanged and testCatFluxSigma share a lot of common code (especially the first two).

            I have a small concern that by constructing the task in setUp, you discourage tests from constructing the task with a different configuration.

            Show
            rowen Russell Owen added a comment - - edited Thank you very much for writing such thorough tests. Overall this looks good. My main suggestion is to try to factor out common code (e.g. in to separate methods or functions). Benefits include: If you find a bug you only have to fix it in one place It usually makes your intent clearer by highlight important differences. Please make sure every method (and class and free function) has a doc string explaining what it is doing. Most of your test methods are missing this. I am nervous about using assertEqual to compare numeric values that should be the same. Unless you have a very strong reason to expect floats to be identical I recommend that you use assertAlmostEqual instead, with a suitable tolerance. Also, this is picky, but please provide a space after the leading "#" in Python comments. It makes them easier to read. A few specific bits: testMeasureApCorr.py testApCorrMapKeys : I don't think it is safe to assume an order for the keys (though I may be wrong). If order is not assured then this is safer: self.assertEqual(set(struct.apCorrMap.keys()), set(key_names)) testTooFewSources : I don't understand what this test is doing (though a method doc string would probably explain). And it's testing zero sources, which is a stretch for the term "too few". testSourceNotUsed : I think this test would be more believable if the source you add had data associated with it (including a centroid that is presumably outside the bbox). testApertureMeasOnes and testApertureMeasTens are one instance of similar code that could be factored out into common code. Please run pyflakes and fix the warnings, including an imported package that is not used and setting variables that are not used. In some cases a line can be deleted; in others perhaps you want to call a function to be sure it doesn't raise an exception, in which case don't assign the returned result to a variable. testApplyApCorr.py testFailureFlagged , testSuccessUnflagged , testCatFluxUnchanged and testCatFluxSigma share a lot of common code (especially the first two). I have a small concern that by constructing the task in setUp, you discourage tests from constructing the task with a different configuration.
            Hide
            krughoff Simon Krughoff added a comment -

            Ian Sullivan Is this merged? If not, can we get it merged and close this out. Then we can close this epic.

            Show
            krughoff Simon Krughoff added a comment - Ian Sullivan Is this merged? If not, can we get it merged and close this out. Then we can close this epic.
            Hide
            sullivan Ian Sullivan added a comment -

            Simon Krughoff It is now merged and marked Done.

            Show
            sullivan Ian Sullivan added a comment - Simon Krughoff It is now merged and marked Done.
            Hide
            jbosch Jim Bosch added a comment - - edited

            Ian Sullivan, next time, please make sure you merge to master with --no-ff (or by pressing the GitHub PR merge button). Nothing we can do about it now for on this branch, and it's far from the first time it's happened, so don't feel bad about it, but something to watch out for.

            Show
            jbosch Jim Bosch added a comment - - edited Ian Sullivan , next time, please make sure you merge to master with --no-ff (or by pressing the GitHub PR merge button). Nothing we can do about it now for on this branch, and it's far from the first time it's happened, so don't feel bad about it, but something to watch out for.
            Hide
            krughoff Simon Krughoff added a comment -

            Sorry both. I should have mentioned that.

            Show
            krughoff Simon Krughoff added a comment - Sorry both. I should have mentioned that.

              People

              • Assignee:
                sullivan Ian Sullivan
                Reporter:
                rowen Russell Owen
                Reviewers:
                Russell Owen
                Watchers:
                Ian Sullivan, Jim Bosch, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel