# 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:
• Labels:
• Story Points:
12
• Sprint:
Measurement-S14-2
• Team:
Data Release Production

#### 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.

#### Activity

Hide
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
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
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
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
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
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
Jim Bosch added a comment -

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
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
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
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:
Perry Gee
Reporter:
Jim Bosch
Reviewers:
Jim Bosch
Watchers:
Jim Bosch, Perry Gee
0 Vote for this issue
Watchers:
2 Start watching this issue

#### Dates

Created:
Updated:
Resolved:

#### Jenkins

No builds found.