Open jamesohortle opened 5 years ago
Begin the review process for the merge.
TODO:
- Makes types more restrictive (remove all
Any
from .pyi and .py files, determine if typesInitDict
andNestedInitDict
are correct).- Think of better names for
InitDict
,NestedInitDict
. (ObservationSequence
,HistoricalObservationSequence
, ...?)- Provide type stubs (.pyi files) to typeshed.
- Rethink initializations of types
InitDict
,NestedInitDict
. (What is purpose of usingNone
? What is purpose of pre-allocating dictionaries/lists/etc. for a dynamic language?) Originally posted by @jamesohortle in #10
Since None
is structurally important, it may be worthwhile to invest in the following idea.
Idea:
class EmissionSequence: ...
and use that as the type for InitDict
, NestedInitDict
, etc. Since emission sequences seem to be quite stateful objects, this will probably make their manipulation more robust (we can expose an API for these objects). Of course, this comes with its own bag of complexity which is something that should be kept to a minimum.Idea:
from numbers import Real
and use Real
as the type of the values in the sequences (avoids use of Numeric = Union[int, float]
? However: PEP484 and python/mypy#3186... Should I just use float
instead? Seems hackey so perhaps an alias is clearer?Index
type.Idea: (EDIT: This is Python >=3.8 only ðŸ˜)
from typing_extensions import Final, final
(final
is decorator; use above "static
" functions, classes that should not be inherited from, etc), Final
is a type (like List
) that you use for constants (i.e. MAGIC_NUMBER: Final = 42
; no need to say int
or anything since the type will be inferred).I don't know if you saw, but I pulled out a lot of my own number typing work-arounds into their own package called numerary
. If this journey lines up with your own, numerary
could be an intermediary solution until python/mypy#3186 is fixed. (Alternatively, the techniques used therein might provide inspiration if you can't take a dependency.) Happy to consult here, if helpful. Apologies if this is a distraction.
@jamesohortle the code as it stands is a strong improvement to the codebase. I'm happy for you to merge into the dev branch now, and put those TODO items in an issue for enhancement in the future. Also, the
None
states have a big structural importance: they serve as the pseudo 'aggregate' state, which represents the infinity of states which are not observed in the model's current state. We can't replace them completely, but maybe we can make a reserved state label so avoid the type conflicts.Originally posted by @jamesross2 in https://github.com/jamesross2/Bayesian-HMM/pull/10#issuecomment-529074266