Ticket #2112 (closed defect)

Opened 7 years ago

Last modified 7 years ago

make afw::table type strings consistent with template name conventions elsewhere

Reported by: jbosch Owned by: jbosch
Priority: normal Milestone:
Component: afw Keywords:
Cc: rhl, rowen, bick, cloomis, price, jbosch Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:

not applicable

Description

The type strings used by afw::table to label C++ template types in Python and FITS (I4, I8, F4, F8) do not match the conventions used elsewhere.

In addition, using angle brackets for compound types with these strings ("Moments<F8>") means they can't be used as Python identifiers, which is inconvenient.

The conventions used by afw::image aren't sufficient to describe the broader range of types in afw::table, so we need to decide on some new conventions before we can implement them. We will also need to decide how we want to handle the distinction between int and int32_t and the various 64-bit integer types.

These changes will be a source of backwards incompatibility, but that will only get worse if we let the inconsistency linger. Jim is sorry he did it wrong the first time.

Change History

comment:1 Changed 7 years ago by DefaultCC Plugin

  • Cc rhl, rowen, bick, cloomis, price, jbosch added

comment:2 Changed 7 years ago by jbosch

  • Status changed from new to assigned

Making these changes will also make it much easier to complete the unit tests for #2111, so that should be done as part of this ticket as well. In particular, we need to test that the fast accessors for array and geometry field types work, once key.getTypeString() returns the same value used to name the accessors (and for that to be the case we need to get rid of the angle brackets).

comment:3 Changed 7 years ago by jbosch

RHL has suggested just adding aliases for float->"F", double->"D", and int32->"I" to the existing typedefs. That doesn't break backward compatibility.

However, I'd like to also remove the angle brackets, and that does break backward compatibility. And if I add int64->"L", I can represent the full range of simple types in afw::table without needing redundant I4, I8, F4, F8 names. That suggests that I should just remove those names and deal with the backwards-compatibility now rather than continue to have two ways to do (almost) everything.

comment:4 Changed 7 years ago by jbosch

It turns out none of these changes break backwards compatibility nearly as much as I thought, because they don't affect the FITS schema description at all.

I think I'll have to change a little bit of Python code in meas/* and pipe_tasks, but I think that's worth doing to improve the consistency.

comment:5 Changed 7 years ago by jbosch

  • Status changed from assigned to inTicketWork

comment:6 Changed 7 years ago by jbosch

  • Owner changed from jbosch to ktl
  • Status changed from inTicketWork to inStandardsReview
  • reviewstatus changed from notReady to needsReview

Ticket is now ready for review. Lots of changes, but it's basically just regular expression substitution and the new test code for #2111. Aside from afw, there was also a two-character change in pipe_tasks. No other packages were affected.

comment:7 Changed 7 years ago by ktl

Looks like more changes will be needed in pipe_tasks tickets/2108...

comment:8 Changed 7 years ago by ktl

  • Owner changed from ktl to srp
  • Status changed from inStandardsReview to inTicketWork

Assigning to srp.

comment:9 Changed 7 years ago by srp

  • Owner changed from srp to jbosch

I took a look at the changes, at it all looks fine to me except for one file:

doc/table.dox

contains references to the old I4, I8, F4, F8, etc. So, when doc/html/afw_table.html gets generated, the old naming scheme is still there.

Other than that, looks OK to me.

comment:10 Changed 7 years ago by jbosch

  • Status changed from inTicketWork to closed
  • reviewstatus changed from needsReview to reviewed

Documentation fixed. Now merged to master.

I have also updated ap, meas_astrom, obs_lsstSim, pipe_tasks, and testing_pipeQA to reflect these changes.

Note: See TracTickets for help on using tickets.