DM-43514: Fix bug in APDB bit flag unpacking #201
Conversation
| for bit_idx, (bit_name, dtypes) in enumerate(bit_names_types): | ||
| masked_bits = np.bitwise_and(input_flag_values, 2**bit_idx) | ||
| output_flags[bit_name] = masked_bits | ||
| for name in self.output_flag_columns[flag_name]: |
There was a problem hiding this comment.
You're basically iterating over column names, right? This might be clearer if the iterable were, accordingly, columnName, here and in the list comp above, as it is in the flagExists method below
Of course, camelCase vs snake_case is rearing its head, maybe it should be column_name throughout...
There was a problem hiding this comment.
They're the name of the thing in the dictionary, so it's just name. It's generally better to keep iterator variables short.
And yeah, Chris was unconsistent with camel vs. snakes. 🤷
| bit_names_types = self.output_flag_columns[flag_name] | ||
| output_flags = np.zeros(len(input_flag_values), dtype=bit_names_types) | ||
| output_flags = np.zeros(len(input_flag_values), | ||
| dtype=[(name, bool) for name in self.output_flag_columns[flag_name]]) |
There was a problem hiding this comment.
I figured out what this does after falling down a dtype rabbit hole, but, possibly silly question, aren't they all required (by virtue of being flags) to just be bool? Is there a reason we bend over backwards to assign the dtypes one at a time?
There was a problem hiding this comment.
I have no idea why Chris originally made this a dict of tuples of (name, bool), because they're all booleans because this is a bit field. That's why I changed that part of it.
I put in a dtype for the numpy array, so that it is a structured array with column names, each one a bool.
| output_flags[bit_name] = masked_bits | ||
| for name in self.output_flag_columns[flag_name]: | ||
| masked_bits = np.bitwise_and(input_flag_values, | ||
| 2**self.output_flag_columns[flag_name][name]) |
There was a problem hiding this comment.
I think this could be clearer if you defined bit_idx = self.output_flag_columns[flag_name][name] first and then set masked_bits using bit_idx, but I don't insist on it.
There was a problem hiding this comment.
It's not bit_idx, though, it's the bit. It's not an index.
* We explicitly number the bits, so enumerating them gives the wrong answer if bits are removed from the map. * Change test flag map to trigger bug. * Fix explicit numeric tests, given new flag map.
|
I tried to cleanup some of the docstrings to help clarify |
parejkoj
force-pushed
the
tickets/DM-43514
branch
from
2fdc875
to
84c3b82
Compare
No description provided.