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

Cleanup SdssShape

    Details

    • Story Points:
      8
    • Sprint:
      Science Pipelines DM-W15-4, Science Pipelines DM-W15-5, Science Pipelines DM-S15-1, Science Pipelines DM-S15-2
    • Team:
      Data Release Production

      Description

      We should do a comprehensive cleanup of the SdssShapeAlgorithm class. This includes removing the SdssShapeImpl interface (never supposed to have been public, but it became public) from other code that uses it, and integrating this code directly into the algorithm class.

      We should also ensure that the source from which the algorithm is derived is clearly cited – that's Bernstein and Jarvis (2002, http://adsabs.harvard.edu/abs/2002AJ....123..583B); see also DM-2304.

        Attachments

          Issue Links

            Activity

            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Epic Link DM-1100 [ 13907 ]
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Rank Ranked lower
            jbosch Jim Bosch made changes -
            Labels AppsPlanning Measurement SciencePipelines
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Story Points 4 2
            jbosch Jim Bosch made changes -
            Assignee Perry Gee [ pgee ] Jim Bosch [ jbosch ]
            jbosch Jim Bosch made changes -
            Sprint Science Pipelines DM-W15-4 [ 120 ]
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-W15-4 [ 120 ] Science Pipelines DM-W15-4, Science Pipelines DM-W15-5 [ 120, 129 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            swinbank John Swinbank made changes -
            Epic Link DM-1100 [ 13907 ] DM-1769 [ 15588 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-W15-4, Science Pipelines DM-W15-5 [ 120, 129 ] Science Pipelines DM-W15-4, Science Pipelines DM-W15-5, Science Pipelines DM-S15-1 [ 120, 129, 140 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            Hide
            jbosch Jim Bosch added a comment -

            This is finally ready for review. The scope of this ticket ballooned a bit. The original goal was just to refactor SdssShape and GaussianFlux to remove the SdssShapeImpl intermediary. Along the way, I realized the tests for those two algorithms weren't strict enough to make me feel safe modifying them, so I put some effort into improving the tests. That led to me rewriting the test utility code, which led to me rewriting the tests for all the other algorithms. And that uncovered some bugs in those other algorithms, which I also fixed.

            In the end, I did end up cleaning up the two original algorithms a bit, and I removed SdssShapeImpl, but I didn't go as far as I might have in cleaning up the internal implementations because I felt maintaining continuity with the old code was important for the guts of the algorithms.

            The git log should be able to tell you the details of the rest. As we discussed earlier, I did also remove the additional unused test utility you added earlier (in the last commit on that branch). I'm happy to revert that change if you think that code will be useful going forward.

            There are changes on tickets/DM-1161 branches of meas_base, meas_algorithms, meas_modelfit, and the demo package, but the vast majority of the the changes are in meas_base. Rather than paste the git diffs here, I'll just create the PRs for the review myself.

            Show
            jbosch Jim Bosch added a comment - This is finally ready for review. The scope of this ticket ballooned a bit. The original goal was just to refactor SdssShape and GaussianFlux to remove the SdssShapeImpl intermediary. Along the way, I realized the tests for those two algorithms weren't strict enough to make me feel safe modifying them, so I put some effort into improving the tests. That led to me rewriting the test utility code, which led to me rewriting the tests for all the other algorithms. And that uncovered some bugs in those other algorithms, which I also fixed. In the end, I did end up cleaning up the two original algorithms a bit, and I removed SdssShapeImpl, but I didn't go as far as I might have in cleaning up the internal implementations because I felt maintaining continuity with the old code was important for the guts of the algorithms. The git log should be able to tell you the details of the rest. As we discussed earlier, I did also remove the additional unused test utility you added earlier (in the last commit on that branch). I'm happy to revert that change if you think that code will be useful going forward. There are changes on tickets/ DM-1161 branches of meas_base, meas_algorithms, meas_modelfit, and the demo package, but the vast majority of the the changes are in meas_base. Rather than paste the git diffs here, I'll just create the PRs for the review myself.
            jbosch Jim Bosch made changes -
            Reviewers Perry Gee [ pgee ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            pgee Perry Gee added a comment -

            I am attempting to get through this long review, and while it is going fine, it will not be done in the 2 days I was supposed to be. Since it came on Friday night and I am preparing for a trip over spring break, I am having to do it when I can.

            If if needs to be done quickly, please let me know and I will see if any of it can be detached. It actually seems like 2 or 3 tickets, anyway. If you can wait until Thursday, I should be able to get done.

            Show
            pgee Perry Gee added a comment - I am attempting to get through this long review, and while it is going fine, it will not be done in the 2 days I was supposed to be. Since it came on Friday night and I am preparing for a trip over spring break, I am having to do it when I can. If if needs to be done quickly, please let me know and I will see if any of it can be detached. It actually seems like 2 or 3 tickets, anyway. If you can wait until Thursday, I should be able to get done.
            Hide
            jbosch Jim Bosch added a comment -

            Thursday is fine. If you can make your comments on the actual lines of code rather than on the commits, though, I think that would make it easier for both of us in the end.

            Show
            jbosch Jim Bosch added a comment - Thursday is fine. If you can make your comments on the actual lines of code rather than on the commits, though, I think that would make it easier for both of us in the end.
            swinbank John Swinbank made changes -
            Link This issue relates to DM-2304 [ DM-2304 ]
            swinbank John Swinbank made changes -
            Description We should do a comprehensive cleanup of the SdssShapeAlgorithm class. This includes removing the SdssShapeImpl interface (never supposed to have been public, but it became public) from other code that uses it, and integrating this code directly into the algorithm class. We should do a comprehensive cleanup of the SdssShapeAlgorithm class. This includes removing the SdssShapeImpl interface (never supposed to have been public, but it became public) from other code that uses it, and integrating this code directly into the algorithm class.

            We should also ensure that the source from which the algorithm is derived is clearly cited -- that's Bernstein and Jarvis (2002, http://adsabs.harvard.edu/abs/2002AJ....123..583B); see also DM-2304.
            swinbank John Swinbank made changes -
            Link This issue is duplicated by DM-2304 [ DM-2304 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-2304 [ DM-2304 ]
            Hide
            jbosch Jim Bosch added a comment -

            Paul Price, can you take on some of this review, specifically the code in python/meas/base/tests.py? Perry Gee is handling the rest, but this is a big ticket, and that file is a big chunk that should be separately-digestible. This is all on meas_base tickets/DM-1161, and in your case, specifically on this commit.

            Show
            jbosch Jim Bosch added a comment - Paul Price , can you take on some of this review, specifically the code in python/meas/base/tests.py ? Perry Gee is handling the rest, but this is a big ticket, and that file is a big chunk that should be separately-digestible. This is all on meas_base tickets/ DM-1161 , and in your case, specifically on this commit .
            jbosch Jim Bosch made changes -
            Reviewers Perry Gee [ pgee ] Paul Price, Perry Gee [ price, pgee ]
            Hide
            pgee Perry Gee added a comment -

            I am going to mark this "review complete", even though I did not review tests.py or the new NoiseReplacer test. I asked Jim to spawn these off. I meant to look at the NoiseReplacer test and see if it was derived from the one in meas_algorithms, but I have simply run out of time.

            In general, the tests are much more thorough than the "Simple" tests which were added when the algorithms were moved. I really only have a few general comments, not presented in line-by-line comments in GitHub.

            The fact that the same test setup code in tests.py is used for almost all the tests is a great savings in coa.lsstcorp.org/de, but I am a little concerned that the same basic test setup is being used over and over again. But if we add good system tests (like extended sdss_demo tests) that will probably be fine.

            I have the feeling that tolerances are being tuned to tests. That was certainly the case when I moved the tests. I see that a test over multiple realizations has been added to several of the tests, which will tend to average out both the random number generator issues and the fact that specific test cases need different tolerances, so things are better than they were before. But it would be nice to know that the tolerance being used in both single tests and averaged multiple tests are reasonable for the algorithm in question.

            Show
            pgee Perry Gee added a comment - I am going to mark this "review complete", even though I did not review tests.py or the new NoiseReplacer test. I asked Jim to spawn these off. I meant to look at the NoiseReplacer test and see if it was derived from the one in meas_algorithms, but I have simply run out of time. In general, the tests are much more thorough than the "Simple" tests which were added when the algorithms were moved. I really only have a few general comments, not presented in line-by-line comments in GitHub. The fact that the same test setup code in tests.py is used for almost all the tests is a great savings in coa.lsstcorp.org/de, but I am a little concerned that the same basic test setup is being used over and over again. But if we add good system tests (like extended sdss_demo tests) that will probably be fine. I have the feeling that tolerances are being tuned to tests. That was certainly the case when I moved the tests. I see that a test over multiple realizations has been added to several of the tests, which will tend to average out both the random number generator issues and the fact that specific test cases need different tolerances, so things are better than they were before. But it would be nice to know that the tolerance being used in both single tests and averaged multiple tests are reasonable for the algorithm in question.
            pgee Perry Gee made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-W15-4, Science Pipelines DM-W15-5, Science Pipelines DM-S15-1 [ 120, 129, 140 ] Science Pipelines DM-W15-4, Science Pipelines DM-W15-5, Science Pipelines DM-S15-1, Science Pipelines DM-S15-2 [ 120, 129, 140, 151 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Story Points 2 8
            Hide
            jbosch Jim Bosch added a comment -

            Just putting this back into "In Review", since that's where it should be - Perry's part of the review is done, but Paul's (tests.py) is not.

            Show
            jbosch Jim Bosch added a comment - Just putting this back into "In Review", since that's where it should be - Perry's part of the review is done, but Paul's (tests.py) is not.
            jbosch Jim Bosch made changes -
            Reviewers Paul Price, Perry Gee [ price, pgee ] Paul Price [ price ]
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            Hide
            price Paul Price added a comment -

            Review done on github. Besides some itty-bitties, I recommend consolidating repeated code.

            Show
            price Paul Price added a comment - Review done on github. Besides some itty-bitties, I recommend consolidating repeated code.
            price Paul Price made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            jbosch Jim Bosch added a comment -

            I believe I've now addressed all review comments (either via changes in the code or replies on the GitHub PR), and I've squashed new commits into old ones as appropriate. Just waiting on a buildbot run before merging.

            Show
            jbosch Jim Bosch added a comment - I believe I've now addressed all review comments (either via changes in the code or replies on the GitHub PR), and I've squashed new commits into old ones as appropriate. Just waiting on a buildbot run before merging.
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Paul Price
                Watchers:
                Jim Bosch, John Swinbank, Paul Price, Perry Gee
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel