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

ensure pipe_tasks, obs*, and other packages are compatible with meas_base

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_tasks
    • Labels:

      Description

      The switch to meas_base will involve changing the names of most measurement algorithms, as we're using a new naming convention that provides more traceability. This will break downstream code that:

      • uses field names instead of slots to access measurements
      • reads or modifies the list of configured-to-run algorithms.

      Whenever possible, we should fix the former by converting them to use slots, as this will automatically provide backwards compatibility. I'm not yet sure how to handle the latter; the easiest solution would be to give up on full backwards-compatibility with meas_algorithms, but we might be able to find some way to make the old names aliases to the new ones during the deprecation period.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            More meas_algorithms

            I think I'd actually be okay with just switching SecondMomentStarSelector over to the new framework entirely, and removing support for the old one, at least if it still passes its own unit tests when using the new framework. Is there any reason you can think of why we'd still need the old one here? The main reason to keep the old one around is that we haven't quite reimplemented all the algorithms or fixed up the error-handling, but I don't think either of those are concerns here.

            Show
            jbosch Jim Bosch added a comment - More meas_algorithms I think I'd actually be okay with just switching SecondMomentStarSelector over to the new framework entirely, and removing support for the old one, at least if it still passes its own unit tests when using the new framework. Is there any reason you can think of why we'd still need the old one here? The main reason to keep the old one around is that we haven't quite reimplemented all the algorithms or fixed up the error-handling, but I don't think either of those are concerns here.
            Hide
            jbosch Jim Bosch added a comment - - edited

            meas_astrom

            "classification_photometric" is another case where we probably want to add package/class information to the field name, but I'm not sure if we want to do it before we have aliases. It'd probably be okay in this case, as I don't think it has any downstream users. And now that I think about it, I think "classification" is not the right prefix for this; it's more similar to the flags set in MeasurePsfTask than those set by the Classification algorithm; maybe "astrom_PhotoCal_used", instead?

            Show
            jbosch Jim Bosch added a comment - - edited meas_astrom "classification_photometric" is another case where we probably want to add package/class information to the field name, but I'm not sure if we want to do it before we have aliases. It'd probably be okay in this case, as I don't think it has any downstream users. And now that I think about it, I think "classification" is not the right prefix for this; it's more similar to the flags set in MeasurePsfTask than those set by the Classification algorithm; maybe "astrom_PhotoCal_used", instead?
            Hide
            jbosch Jim Bosch added a comment -

            meas_deblender

            Let's change all the version 1 keys here so we use camel-case instead of dashes (e.g. "deblend_psfCenter" instead of "deblend_psf-center"). I also think we'll want to rename all of these to e.g. "deblender_SourceDeblendTask_*" once we have aliases, as I've discussed for a few other fields above.

            Show
            jbosch Jim Bosch added a comment - meas_deblender Let's change all the version 1 keys here so we use camel-case instead of dashes (e.g. "deblend_psfCenter" instead of "deblend_psf-center"). I also think we'll want to rename all of these to e.g. "deblender_SourceDeblendTask_*" once we have aliases, as I've discussed for a few other fields above.
            Hide
            jbosch Jim Bosch added a comment -

            pipe_tasks

            Minor quibble, but we should probably fix it here: the "TableVersion" class variable (defined in meas_base and meas_algorithms, but I didn't notice it there as it wasn't part of this issue's changeset) should be lowercase, i.e. "tableVersion". I can understand how you'd be confused; "_DefaultName" should also be lowercase, but is now tricker to change, and "ConfigClass" is only uppercase because it refers to a type (and hence behaves more like a typedef than a traditional variable - it's a thorny issue).

            "calib_detected" - another name that needs to be more descriptive, but perhaps not yet. This is shaping up into an issue that probably merits a larger group discussion. I'll start writing an RFC email to the list.

            processCoadd.py: switch dashes to camelCase here, just as in meas_deblender.

            calibrate.py:142: this sort of multi-line if statement must occupy four lines:

            if self.tableVersion == 0:
                separator = "."
            else:
                separator =  "_"

            You can also use a conditional statement in this particular case:

            separator = "_" if self.tableVersion > 0 else "."

            astrometry.py, measurePsf.py: looks fine, except for more of those names we need to figure out what to do with.

            Show
            jbosch Jim Bosch added a comment - pipe_tasks Minor quibble, but we should probably fix it here: the "TableVersion" class variable (defined in meas_base and meas_algorithms, but I didn't notice it there as it wasn't part of this issue's changeset) should be lowercase, i.e. "tableVersion". I can understand how you'd be confused; "_DefaultName" should also be lowercase, but is now tricker to change, and "ConfigClass" is only uppercase because it refers to a type (and hence behaves more like a typedef than a traditional variable - it's a thorny issue). "calib_detected" - another name that needs to be more descriptive, but perhaps not yet. This is shaping up into an issue that probably merits a larger group discussion. I'll start writing an RFC email to the list. processCoadd.py: switch dashes to camelCase here, just as in meas_deblender. calibrate.py:142: this sort of multi-line if statement must occupy four lines: if self .tableVersion = = 0 : separator = "." else : separator = "_" You can also use a conditional statement in this particular case: separator = "_" if self .tableVersion > 0 else "." astrometry.py, measurePsf.py: looks fine, except for more of those names we need to figure out what to do with.
            Hide
            jbosch Jim Bosch added a comment -

            Perry, I'm done with the review of the code in its current state, but I'd like to chat about next steps; we need to make a big announcement to the list before merging this to master (at least if we're changing the defaults, and I'd still like to do that), and we may want to consider doing some alias work before we get this merged in. Please find me on HipChat sometime; I should be around this week.

            Show
            jbosch Jim Bosch added a comment - Perry, I'm done with the review of the code in its current state, but I'd like to chat about next steps; we need to make a big announcement to the list before merging this to master (at least if we're changing the defaults, and I'd still like to do that), and we may want to consider doing some alias work before we get this merged in. Please find me on HipChat sometime; I should be around this week.

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.