DM-31924: make data ID packing configurable #63
Conversation
I'm not sure this was ever used by daf_butler code that made it to the 'main' branch, and it definitely hasn't been used in a long while.
TallJimbo
force-pushed
the
tickets/DM-31924
branch
2 times, most recently
from
3b40a0e
to
47e07b8
Compare
There was a problem hiding this comment.
This does look like a better approach, but I'm nervous about some of the explicit and implicit logic.
I think pycov would be not very happy about the series of nested ifs with the existing tests; I don't know how much of a pain it would be to get full coverage of the __init__, but at least mapping out a chunk of that space would make me more comfortable.
| a fixed set of bands defined in this class, but this should be avoided | ||
| in new code. | ||
| n_bands : `int` or `None`, optional | ||
| The number of bands to leave room for in the packed ID. If `None`, |
There was a problem hiding this comment.
Maybe worth mentioning here why one might want n_bands != len(bands)? > makes sense to me, to leave room for future changes, but should < be a logic or value error? I didn't make the connection as to why both of these could be passed until I looked at the logic of the code below.
There was a problem hiding this comment.
I could see < being useful especially when it comes from the config interface, as a way to more easily have single consistent mapping of which sometimes only a subset is used (e.g. broadband + narrowband filters).
There was a problem hiding this comment.
I think that's worth a sentence in this docstring, because I don't feel it's an obvious usage.
python/lsst/skymap/packers.py
Outdated
| a fixed set of bands defined in this class, but this should be avoided | ||
| in new code. | ||
| a fixed set of bands defined in this class. When calling code can | ||
| enumerate the bands it is likely to see providing an explict mapping is |
There was a problem hiding this comment.
"it is likely to see providing"?
There was a problem hiding this comment.
comma makes all the difference; should be "is likely to see, providing"
TallJimbo
force-pushed
the
tickets/DM-31924
branch
from
f5201d8
to
3208621
Compare
No description provided.