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

Implement RFC-498: homogenize naming of calibration flags

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_tasks
    • Labels:
      None
    • Story Points:
      6
    • Sprint:
      DRP F18-2, DRP F18-3
    • Team:
      Data Release Production

      Description

      We currently have the following set of names for the calib_* flags that get propagated to source catalogs:

      • calib_detected
      • calib_psfCandidate
      • calib_psfUsed
      • calib_psf_reserved (changed from calib_psfReserved on DM-12207 in this commit)
      • calib_astrometryUsed
      • calib_photometry_used (changed from calib_photometryUsed on DM-12207, in this commit)
      • calib_photometry_reserved (changed from calib_photometryReserved on DM-12207 in this commit)

      I.e. there is a mixture of underscore and camelCase for the third (final) "descriptor", making it difficult to remember which standard to use when trying to select on a given flag.  These should all be homogenized and, given the recent changes in this direction noted above, I believe the preferred choice is the underscore standard.

      A change in the flag names will affect any current user scripts that select on these flags.  To prevent scripts from breaking, it may be desirable to set a (temporary?) alias (although this was not done along with the DM-12207 changes and there was no uproar that I'm aware of).

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Schema naming convention documentation on when to use CamelCase vs. underscores either never existed outside or had been lost to the depths of time.  This has been fixed on DM-14998.  Relevant excerpt:

            A good rule of thumb is that if two words are not individually meaningful (or mean something different when separated), join them with CamelCase.

             

            Show
            jbosch Jim Bosch added a comment - Schema naming convention documentation on when to use CamelCase vs. underscores either never existed outside or had been lost to the depths of time.  This has been fixed on DM-14998 .  Relevant excerpt: A good rule of thumb is that if two words are not individually meaningful (or mean something different when separated), join them with CamelCase.  
            Hide
            lauren Lauren MacArthur added a comment -

            Would you mind giving this a look?  In particular, I'd like to know if you are happy with the accommodation I made in jointcal for the test that uses previously processed catalogs that have the old names and one is used in the FlaggedSourceSelector.  The other option I considered is a try/except in the FlaggedSourceSelector itself along the lines:

                    try:
                        key = sourceCat.schema.find(self.config.field).key
                    except NoResults:
                        # Backwards compatibility with old (pre-DM-14997) source catalogs
                        oldFieldName = self.config.field
                        if any(subStr in oldFieldName for subStr in ("_used", "_reserved", "_candidate")):
                            undIndex = oldFieldName.rfind("_")
                            oldFieldName = replace(oldFieldName[undIndex: undIndex + 2],
                                                   oldFieldName[undIndex + 1].capitalize())
                            key = sourceCat.schema.find(oldFieldName).key
                            self.log.warn(("Your catalog has old, pre-RFC-498, calib flag naming ({} is now named "
                                           "{}).  Consider regenerating with an up-to-date stack").
                                          format(oldFieldName, self.config.field))
              
            

            but I think just editing the jointcal test is less invasive (and is the only place that uses the FlaggedSourceSelector, as far as I can tell).

            I've already had a successful Jenkins lsst_distrib + ci_hsc run, but I just had to rebase to some updates on master, so a new one is running now just to be sure.

            Also, as soon as this lands, I'm going to announce it as widely as possible via a community post and pings to slack channels (dm-square and desc-ssim/dc2 in particular) and a few individuals where I know (via an all of GitHub search on calib_psfUsed) there are users with notebooks that use some of the old flag names. I will include in the post an example piece of code that would add aliases to a catalog when read in to keep things backwards compatible for anyone wanting to look at catalogs processed with the old and/or new naming (I'm working on that now for pipe_analysis on DM-15109).

            Show
            lauren Lauren MacArthur added a comment - Would you mind giving this a look?  In particular, I'd like to know if you are happy with the accommodation I made in jointcal for the test that uses previously processed catalogs that have the old names and one is used in the FlaggedSourceSelector .  The other option I considered is a try/except in the  FlaggedSourceSelector itself along the lines: try : key = sourceCat.schema.find( self .config.field).key except NoResults: # Backwards compatibility with old (pre-DM-14997) source catalogs oldFieldName = self .config.field if any (subStr in oldFieldName for subStr in ( "_used" , "_reserved" , "_candidate" )): undIndex = oldFieldName.rfind( "_" ) oldFieldName = replace(oldFieldName[undIndex: undIndex + 2 ], oldFieldName[undIndex + 1 ].capitalize()) key = sourceCat.schema.find(oldFieldName).key self .log.warn(( "Your catalog has old, pre-RFC-498, calib flag naming ({} is now named " "{}). Consider regenerating with an up-to-date stack" ). format (oldFieldName, self .config.field)) but I think just editing the jointcal test is less invasive (and is the only place that uses the FlaggedSourceSelector , as far as I can tell). I've already had a successful Jenkins lsst_distrib + ci_hsc run , but I just had to rebase to some updates on master, so a new one is running now just to be sure. Also, as soon as this lands, I'm going to announce it as widely as possible via a community post and pings to slack channels (dm-square and desc-ssim/dc2 in particular) and a few individuals where I know (via an all of GitHub search on calib_psfUsed ) there are users with notebooks that use some of the old flag names. I will include in the post an example piece of code that would add aliases to a catalog when read in to keep things backwards compatible for anyone wanting to look at catalogs processed with the old and/or new naming (I'm working on that now for pipe_analysis on DM-15109 ).
            Hide
            Parejkoj John Parejko added a comment -

            This looks fine.

            I like your solution for how to deal with jointcal's flaggedSourceSelector test: it's the simplest and I don't know when we'll reprocess that particular data (it may be a while).

            Show
            Parejkoj John Parejko added a comment - This looks fine. I like your solution for how to deal with jointcal's flaggedSourceSelector test: it's the simplest and I don't know when we'll reprocess that particular data (it may be a while).
            Hide
            lauren Lauren MacArthur added a comment -

            Thanks for the review, John.  It came in just as I was going on vacation, hence the delay in getting this merged.  I just rebased and a Jenkins run passed. However, in a bout of paranoia, I've decided to wait until after the PCW next week to merge this (I fear demo notebooks using latest weekly but pointing to old processed catalogs, etc.)

            Show
            lauren Lauren MacArthur added a comment - Thanks for the review, John.  It came in just as I was going on vacation, hence the delay in getting this merged.  I just rebased and a Jenkins run passed . However, in a bout of paranoia, I've decided to wait until after the PCW next week to merge this (I fear demo notebooks using latest weekly but pointing to old processed catalogs, etc.)
            Hide
            lauren Lauren MacArthur added a comment -

            I rebased to master and reran Jenkins (lsst_distrib + ci_hsc) on Monday.  This run succeded, finished too late for me to merge. I kicked off another Jenkins today (I did not have to rebase any packages) just to be sure. The two centos builds passed and the OS X is almost all the way though, but I'm going to merge now rather than wait for it to finish fearing having to go through this process again.

            Show
            lauren Lauren MacArthur added a comment - I rebased to master and reran Jenkins (lsst_distrib + ci_hsc) on Monday.  This run succeded , finished too late for me to merge. I kicked off another Jenkins today (I did not have to rebase any packages) just to be sure. The two centos builds passed and the OS X is almost all the way though, but I'm going to merge now rather than wait for it to finish fearing having to go through this process again.

              People

              • Assignee:
                lauren Lauren MacArthur
                Reporter:
                lauren Lauren MacArthur
                Reviewers:
                John Parejko
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur, Paul Price, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel