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

Extract and visualize the local and the spatial AL kernel solution coefficients

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ip_diffim
    • Labels:
      None
    • Story Points:
      12
    • Epic Link:
    • Sprint:
      AP S19-5, AP S19-6, AP F19-1
    • Team:
      Alert Production

      Description

      • Examine how to extract the local (fixed) and spatial AL kernel solution as coefficients of the basis functions.
      • Create an overview diagram that captures the implementation concept in the current C++ AL kernel solution ip_diffim code. Summarise the main role(s) of each class.
      • Add a new section into the ip_diffim code notes and include the new diagram. Consider referring to or copy paragraphs from low-level code docstrings like psfMatchTask.py, etc.
      • Add debug plotting for the coefficient visualization with possibly new lsstDebug config options.

        Attachments

          Issue Links

            Activity

            Hide
            gkovacs Gabor Kovacs added a comment - - edited

            Would you please review this code and documentation changes?

            • some of the TODO DM-17458 (edit: number corrected) docstrings were updated separately in DM-19670
            • The use of global keptPlots is to conform to already existing plotting code.
            Show
            gkovacs Gabor Kovacs added a comment - - edited Would you please review this code and documentation changes? some of the TODO DM-17458 (edit: number corrected) docstrings were updated separately in DM-19670 The use of global keptPlots is to conform to already existing plotting code.
            Hide
            mrawls Meredith Rawls added a comment -

            I am willing to review, but want to make sure I have all the tickets and branches straight in my head.

            • You mention DM-17548 above, but that must be a typo for DM-17458 (complete docstrings in ip_diffim), no problem.
            • You point out that DM-19670 (another ip_diffim docstring update ticket) has some of the items originally allocated to DM-17458, that's fine and I will trust the reviewer of that ticket to look at those changes (hi Ian Sullivan!).
            • This ticket appears to have a user branch that has already been merged into master (albeit in an diffimTests, an lsst-dm repo). This does not follow standard DM workflow; the developer guide says user branches must be converted into a ticket branch first and can then go through code review and get merged into master. This is true for lsst organization repos as well as lsst-dm. Is it possible to revert this merge, and then if there is something in it you do want to land on master, I can review that too?

            I will take a look at the documentation and plotting additions in the ip_diffim PR in more detail on GitHub and leave comments over there.

            Is there someplace you could put an example for how to use the various plotting scripts that live in utils.py, and a list of the plots it is currently capable of making? There seem to be lots of potentially useful plotters in there with very minimal documentation. I worry about adding a many more custom ones that are used for just a couple special cases. Perhaps this warrants its own ticket, but it is not obvious at a glance how one would use the new plotKernelCoefficients function you added, much less any of the others.

            Show
            mrawls Meredith Rawls added a comment - I am willing to review, but want to make sure I have all the tickets and branches straight in my head. You mention DM-17548 above, but that must be a typo for DM-17458 (complete docstrings in ip_diffim), no problem. You point out that DM-19670 (another ip_diffim docstring update ticket) has some of the items originally allocated to DM-17458 , that's fine and I will trust the reviewer of that ticket to look at those changes (hi Ian Sullivan !). This ticket appears to have a user branch that has already been merged into master (albeit in an diffimTests , an lsst-dm repo). This does not follow standard DM workflow; the developer guide says user branches must be converted into a ticket branch first and can then go through code review and get merged into master. This is true for lsst organization repos as well as lsst-dm. Is it possible to revert this merge, and then if there is something in it you do want to land on master, I can review that too? I will take a look at the documentation and plotting additions in the ip_diffim PR in more detail on GitHub and leave comments over there. Is there someplace you could put an example for how to use the various plotting scripts that live in utils.py , and a list of the plots it is currently capable of making? There seem to be lots of potentially useful plotters in there with very minimal documentation. I worry about adding a many more custom ones that are used for just a couple special cases. Perhaps this warrants its own ticket, but it is not obvious at a glance how one would use the new plotKernelCoefficients function you added, much less any of the others.
            Hide
            gkovacs Gabor Kovacs added a comment - - edited

            Re lsst-dm/diffimTests: following my predecessor, I took the regular workflow leniently for this repo, also it was "just" a backup of the diagram source files. Anyway, I did the exercise to break the former merge and to turn the branch into a proper PR; this included force pushing to the master which perhaps something we should never do on the "real repositories"...

            Show
            gkovacs Gabor Kovacs added a comment - - edited Re lsst-dm/diffimTests: following my predecessor, I took the regular workflow leniently for this repo, also it was "just" a backup of the diagram source files. Anyway, I did the exercise to break the former merge and to turn the branch into a proper PR; this included force pushing to the master which perhaps something we should never do on the "real repositories"...
            Hide
            mrawls Meredith Rawls added a comment -

            Thanks. I don't think you should ever force push to master but the new PR looks OK, however you managed it  Let me know if you want me to revisit the original ip_diffim PR once you've addressed the comments there.

            As a side note, it helps to keep the PR titles identical to the ticket titles and provide context/summary in the PR comments. So for all PRs for this ticket, the title should be "DM-19443: Extract and visualize the local and the spatial AL kernel solution coefficients". I updated this for you.

            Show
            mrawls Meredith Rawls added a comment - Thanks. I don't think you should ever force push to master but the new PR looks OK, however you managed it  Let me know if you want me to revisit the original ip_diffim PR once you've addressed the comments there. As a side note, it helps to keep the PR titles identical to the ticket titles and provide context/summary in the PR comments. So for all PRs for this ticket, the title should be " DM-19443 : Extract and visualize the local and the spatial AL kernel solution coefficients". I updated this for you.
            Hide
            gkovacs Gabor Kovacs added a comment -

            I addressed your comments:

            • Added an introduction to give context.
            • Typos and trivial misleading sentences fixed.
            • Minor docstring updates.
            • Chart source files are now part of the documentation and served, this was backported to the flowcharts, too.
            • Added link to doxygen start page. As far as I understand, doxygen URLs of classes/methods contain auto generated index elements so may change with future docs builds; maybe better to avoid them.

            The basis functions section naturally would benefit from extension but I think this is beyond this ticket.

            I recognize that at some point perhaps it'd be useful to reword and merge this page and the "Package usage notes" I wrote earlier to get a more coherent technical manual, but I think this work is definitely beyond the present ticket.

            Show
            gkovacs Gabor Kovacs added a comment - I addressed your comments: Added an introduction to give context. Typos and trivial misleading sentences fixed. Minor docstring updates. Chart source files are now part of the documentation and served, this was backported to the flowcharts, too. Added link to doxygen start page. As far as I understand, doxygen URLs of classes/methods contain auto generated index elements so may change with future docs builds; maybe better to avoid them. The basis functions section naturally would benefit from extension but I think this is beyond this ticket. I recognize that at some point perhaps it'd be useful to reword and merge this page and the "Package usage notes" I wrote earlier to get a more coherent technical manual, but I think this work is definitely beyond the present ticket.
            Hide
            mrawls Meredith Rawls added a comment -

            OK, I think it is fine for now! If you can delete the typo (double "calls calls") I will feel better.

            For the future, it is helpful to respond to GitHub comments one-by-one as you resolve them (even if they appear as outdated) so I can tell you did something to address it and didn't just ignore it. The emoji react is good for this if you don't want to leave another comment.

            Show
            mrawls Meredith Rawls added a comment - OK, I think it is fine for now! If you can delete the typo (double "calls calls") I will feel better. For the future, it is helpful to respond to GitHub comments one-by-one as you resolve them (even if they appear as outdated) so I can tell you did something to address it and didn't just ignore it. The emoji react is good for this if you don't want to leave another comment.

              People

              • Assignee:
                gkovacs Gabor Kovacs
                Reporter:
                gkovacs Gabor Kovacs
                Reviewers:
                Meredith Rawls
                Watchers:
                Gabor Kovacs, Meredith Rawls
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel