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

Implement RFC-750

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_tasks
    • Labels:
      None
    • Story Points:
      16
    • Sprint:
      DRP S21a (Dec Jan)
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      Add the `isSimpleLeaf` and `isModelLeaf` flags to catalogs, and have `isPrimary` give the same value as `isSimpleLeaf`.

        Attachments

          Issue Links

            Activity

            Hide
            fred3m Fred Moolekamp added a comment -

            Hi Lauren,
            Would you mind reviewing this for me? This is the setPrimaryFlags ticket we talked about the other day. I refactored the task completely to use vectorized functions, per the discussion that followed this slack comment. I also factored out the different parts of the algorithm so that other code downstream (like pipe_analysis) that might not want to use isPrimary but may want to know about whether or not it is in a given tract, patch, or is a SimpleLeaf or ModelLeaf (per RFC-750).

            Jenkins is building now, and I'll post a comment when it has successfully finished.

            Thanks,
            -Fred

            Show
            fred3m Fred Moolekamp added a comment - Hi Lauren, Would you mind reviewing this for me? This is the setPrimaryFlags ticket we talked about the other day. I refactored the task completely to use vectorized functions, per the discussion that followed this slack comment. I also factored out the different parts of the algorithm so that other code downstream (like pipe_analysis) that might not want to use isPrimary but may want to know about whether or not it is in a given tract, patch, or is a SimpleLeaf or ModelLeaf (per RFC-750 ). Jenkins is building now, and I'll post a comment when it has successfully finished. Thanks, -Fred
            Show
            fred3m Fred Moolekamp added a comment - successful Jenkins build: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/33508/pipeline
            Hide
            lauren Lauren MacArthur added a comment -

            Apologies for the delay on this.  I've left some preliminary comments on the PR.  I'm not quite done, but I hesitate to go further at this point as your response to some of my questions will help me understand if I've properly understood the decisions for the implementation made on the RFC (and thus help guide the rest of my review).

            One validation check I would like to see is if the sources that get labelled as isPrimary end up the same for a meas_deblender run with this branch (you'll see one of my comments that makes me think this may not be the case as is...).

            Show
            lauren Lauren MacArthur added a comment - Apologies for the delay on this.  I've left some preliminary comments on the PR.  I'm not quite done, but I hesitate to go further at this point as your response to some of my questions will help me understand if I've properly understood the decisions for the implementation made on the RFC (and thus help guide the rest of my review). One validation check I would like to see is if the sources that get labelled as isPrimary end up the same for a meas_deblender run with this branch (you'll see one of my comments that makes me think this may not be the case as is...).
            Hide
            lauren Lauren MacArthur added a comment -

            Some more comments on the PR for your consideration.

            Show
            lauren Lauren MacArthur added a comment - Some more comments on the PR for your consideration.
            Hide
            fred3m Fred Moolekamp added a comment -

            Ok, I think that I've addressed all of your latest comments. The main issue was a bug in an older version of the skymap mock ups that didn't place the patches correctly. I've tested this code in a jupyter notebook and everything works as expected now. Let me know if you're still having trouble following the mock ups, but the basic idea is that I wanted a skymap that can be defined by a tract bounding box with a given number of patches that makes it very clear when a source is contained in a tract or patch.

            Show
            fred3m Fred Moolekamp added a comment - Ok, I think that I've addressed all of your latest comments. The main issue was a bug in an older version of the skymap mock ups that didn't place the patches correctly. I've tested this code in a jupyter notebook and everything works as expected now. Let me know if you're still having trouble following the mock ups, but the basic idea is that I wanted a skymap that can be defined by a tract bounding box with a given number of patches that makes it very clear when a source is contained in a tract or patch.
            Hide
            lauren Lauren MacArthur added a comment - - edited

            I was really hoping to feel confident hitting the "Reviewed" button on this round...but I am still a bit puzzled by a few things (as noted on the PR).  We are ssoooo close (and thanks for your patience with an easily confused and sometimes delinquent reviewer...)! 

            While I'm causing you headaches, could I trouble you to also include updating the "wrong-sign" description (and backtick usage) in the documentation here?

            Show
            lauren Lauren MacArthur added a comment - - edited I was really hoping to feel confident hitting the "Reviewed" button on this round...but I am still a bit puzzled by a few things (as noted on the PR).  We are ssoooo close (and thanks for your patience with an easily confused and sometimes delinquent reviewer...)!  While I'm causing you headaches, could I trouble you to also include updating the "wrong-sign" description (and backtick usage) in the documentation here ?
            Hide
            fred3m Fred Moolekamp added a comment -

            Fixed or replied to comments in all of the PR's, and the RFC. Also fixed the doc bug described above.

            Show
            fred3m Fred Moolekamp added a comment - Fixed or replied to comments in all of the PR's, and the RFC. Also fixed the doc bug described above.
            Hide
            lauren Lauren MacArthur added a comment -

            Sorry this has been such a slog, but I've now marked everything as reviewed.  There are a few outstanding naming issues I'm struggling with which I have noted on the PR and the RFC.  If you could give the other watchers a chance to weigh in, that would be great.  As I've also noted, I'm willing to give in if I'm alone on an island on either issue!  I'll leave this up to your discretion and you can then feel free to merge after a final Jenkins + ci_hsc run.

            I also think it would be a good idea to run multiband and the pipe_analysis scripts on the RC2 tracts prior to the next official run so we can update the flag selections on the latter and make sure all looks as you would expect when we do so (we can do this post-merge, but I don't want any more surprises for Eric's next run that we could/should've avoided!).

            Show
            lauren Lauren MacArthur added a comment - Sorry this has been such a slog, but I've now marked everything as reviewed.  There are a few outstanding naming issues I'm struggling with which I have noted on the PR and the RFC.  If you could give the other watchers a chance to weigh in, that would be great.  As I've also noted, I'm willing to give in if I'm alone on an island on either issue!  I'll leave this up to your discretion and you can then feel free to merge after a final Jenkins + ci_hsc run. I also think it would be a good idea to run multiband  and the pipe_analysis scripts on the RC2 tracts prior to the next official run so we can update the flag selections on the latter and make sure all looks as you would expect when we do so (we can do this post-merge, but I don't want any more surprises for Eric's next run that we could/should've avoided!).
            Hide
            fred3m Fred Moolekamp added a comment -

            Thanks Lauren MacArthur. I updated the RFC with new column names `isDeblendedSource` and `isDeblendedModelSource` which will hopefully settle that issue (but chime in if that's not the case). I'll rebase with these last updates and run the 9813 RC2 tract (since that's the one that's most likely to give us any issues), Jenkins, and ci_hsc. That should give people at least a day to update RFC-750 if they see fit, and then I can merge this ticket and you/I can create a ticket that implements the new flags in pipe_analysis so that we can test it before the next full RC2 re-processing.

            Show
            fred3m Fred Moolekamp added a comment - Thanks Lauren MacArthur . I updated the RFC with new column names `isDeblendedSource` and `isDeblendedModelSource` which will hopefully settle that issue (but chime in if that's not the case). I'll rebase with these last updates and run the 9813 RC2 tract (since that's the one that's most likely to give us any issues), Jenkins, and ci_hsc. That should give people at least a day to update RFC-750 if they see fit, and then I can merge this ticket and you/I can create a ticket that implements the new flags in pipe_analysis so that we can test it before the next full RC2 re-processing.
            Hide
            lauren Lauren MacArthur added a comment -

            Thanks, Fred.  Sounds good to me!

            Show
            lauren Lauren MacArthur added a comment - Thanks, Fred.  Sounds good to me!
            Hide
            fred3m Fred Moolekamp added a comment - - edited
            Show
            fred3m Fred Moolekamp added a comment - - edited Successful Jenkins build (including ci_hsc): https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/33789/pipeline
            Hide
            fred3m Fred Moolekamp added a comment -

            RC2 tract 9813 completed successfully and merged branches. Lauren MacArthur feel free to create a ticket to update pipe_analysis to use the new flags set in pipe_tasks and test on the rerun located at `/project/fred3m/rerun/rfc750`.

            Show
            fred3m Fred Moolekamp added a comment - RC2 tract 9813 completed successfully and merged branches. Lauren MacArthur feel free to create a ticket to update pipe_analysis to use the new flags set in pipe_tasks and test on the rerun located at `/project/fred3m/rerun/rfc750`.

              People

              Assignee:
              fred3m Fred Moolekamp
              Reporter:
              fred3m Fred Moolekamp
              Reviewers:
              Lauren MacArthur
              Watchers:
              Fred Moolekamp, Lauren MacArthur
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.