[query] Add AlleleType Enum #14360
[query] Add AlleleType Enum #14360
Conversation
|
Stacked on #14297 |
chrisvittal
force-pushed
the
query/AlleleType
branch
3 times, most recently
from
c2f4bea to
b6abd2b
Compare
chrisvittal
force-pushed
the
query/AlleleType
branch
from
7e5741c to
2d39fbd
Compare
hail/python/hail/ir/ir.py
Outdated
| @@ -56,7 +56,7 @@ class I32(IR): | |||
| @typecheck_method(x=int) | |||
| 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
chrisvittal
force-pushed
the
query/AlleleType
branch
from
6a5d634 to
b31c083
Compare
chrisvittal
force-pushed
the
query/AlleleType
branch
from
6a34c71 to
a739fac
Compare
|
@patrick-schultz bump |
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.