Repository navigation
[query] Add AlleleType Enum - #14360
Conversation
chrisvittal
added
the
stacked PR
|
Stacked on #14297 |
c2f4bea to
b6abd2b
Compare
chrisvittal
removed
the
stacked PR
7e5741c to
2d39fbd
Compare
chrisvittal
left a comment
chrisvittal
left a comment
There was a problem hiding this comment.
I have one unknown about the design right now. See comment for what to do about generating IR with IntEnum values.
| def __init__(self, x): | ||
| super().__init__() | ||
| self.x = x | ||
| self.x = int(x) |
There was a problem hiding this comment.
This conversion is because if we pass an IntEnum to the IR constructors, it will generally be assigned as is. When rendered, we (generally) use format strings (__format__ method, which generally delegates to the __str__ method) to render these items. I would like the __str__ method of AlleleType to be the string that allele_type would return, but then when used as a number, we need to make sure to actually convert it to a number.
I'm not sure what to do here. Let's discuss.
There was a problem hiding this comment.
Seems to me the root of the problem is you've made AlleleType a subtype of int, but its str doesn't act like an int. I'd say it should either fully quack like an int, or you should be forced to call x.value to get the int representation.
If we instead forced calling x.name to get the string name, where would that actually impact users?
There was a problem hiding this comment.
(Note that the default str of IntEnum doesn't return the int representation either, but they fixed that in 3.11, so we can just override it to match the future behavior)
There was a problem hiding this comment.
Thanks for the commentary. I'm going add a pretty_name that matches with the result of allele_type.
There was a problem hiding this comment.
And now we're back to no changes to ir.py
This exposes functionality previously present in the private
`hail.expr.functions._allele_ints` and
`hail.expr.functions._num_allele_type`, used to avoid strings in the
query when determining allele type.
Implement AlleleType as a python [IntEnum].
Replace `_num_allele_type` with the now public `numeric_allele_type`.
As a note to developers of hail, it is _unlikely_ that the enum values
of AlleleType will change, but they are documented as though they could.
CHANGELOG: Exposed previously internal _num_allele_type as
numeric_allele_type and deprecated it. Add new AlleleType
enumeration for users to be able to easily use the values
returned by numeric_allele_type.
[IntEnum]: https://docs.python.org/3/library/enum.html#enum.IntEnum
6a5d634 to
b31c083
Compare
6a34c71 to
a739fac
Compare
|
@patrick-schultz bump |
3 participants
This exposes functionality previously present in the private
hail.expr.functions._allele_intsandhail.expr.functions._num_allele_type, used to avoid strings in the query when determining allele type.Implement AlleleType as a python IntEnum.
Replace
_num_allele_typewith the now publicnumeric_allele_type.As a note to developers of hail, it is unlikely that the enum values of AlleleType will change, but they are documented as though they could.
CHANGELOG: Exposed previously internal
_num_allele_typeasnumeric_allele_typeand deprecated it. Add newAlleleTypeenumeration for users to be able to easily use the values returned bynumeric_allele_type.