Use analytical solver for BitLengthSet instead of numerical computation #66
Conversation
|
@thirtytwobits I tagged you for review but it is not really necessary to dig into the code. Rather, a high-level response like "yup it makes sense" (or don't) is needed. I linked the new docs in the OP post. |
…scovered thanks to PYDSDL_POISON_SLOW_EXPANSION)
pavel-kirienko
force-pushed
the
combinatorial-explosion
branch
from
f0b8fca to
01d44bb
Compare
…repetition operator
pavel-kirienko
force-pushed
the
combinatorial-explosion
branch
from
2a47b2f to
2ff2f64
Compare
|
|
||
| # ======================================== DEPRECATED METHODS ======================================== | ||
|
|
||
| def elementwise_sum_k_multicombinations(self, k: int) -> "BitLengthSet": # pragma: no cover |
There was a problem hiding this comment.
We should add a @deprecated decorator to allow introspection and linting. For example: https://stackoverflow.com/questions/64593550/lint-usages-of-functions-with-deprecated-decorator
There was a problem hiding this comment.
idk it seems like too much work for such little benefit. PyTest will warn about deprecations; PyCharm recognizes them as-is:
|
|
||
| # ======================================== AUXILIARY METHODS ======================================== | ||
|
|
||
| def __eq__(self, other: typing.Any) -> bool: |
There was a problem hiding this comment.
should we mark this as deprecated to avoid continued use of this method?
There was a problem hiding this comment.
I updated its implementation since I wrote the OP post. The new implementation is constant-time but currently, it may yield false positives for sets whose min and max are equal (this is documented). I don't think it deserves deprecation since it is useful, especially in testing.
|
|
||
| # ======================================== SLOW NUMERICAL METHODS ======================================== | ||
|
|
||
| def __iter__(self) -> typing.Iterator[int]: |
There was a problem hiding this comment.
We should mark this and len as deprecated as well. Let's drive all of the slow expansion out of use over time.
There was a problem hiding this comment.
Can't do it yet because the expression evaluation logic depends on numerical expansion.
| raise TypeError("Invalid element for nullary set operator: %r" % x) | ||
|
|
||
| def modulo(self, divisor: int) -> typing.Set[int]: | ||
| return set(map(lambda x: x % divisor, self._value)) |
There was a problem hiding this comment.
nit but if _value is immutable you could cache the set values and just return a copy without redoing the mapping.
There was a problem hiding this comment.
I extracted the caching concern into MemoizationOperator.
* Migrate the C templates to use the new BitLengthSet API from OpenCyphal/pydsdl#66 * Remove unnecessary base dependency on pydsdl * Fix incorrect invocations from the template * Do not unroll fixed-length arrays
* Adopt OpenCyphal/pydsdl#66 * Add complex DSDL type to catch regressions * Do not unroll (de)serialization of fixed-length arrays * Update Nunavut
Fixes #23
This change request slightly alters the BitLengthSet API by adding handles for the underlying solver but the old entries are kept in place to not break existing applications. The old entries are still susceptible to combinatorial explosion so in order to take advantage of the solver, existing applications (Nunavut) should be updated. The update amounts to ditching all calls to
BitLengthSet.__iter__,BitLengthSet.__len__,BitLengthSet.__eq__min,max,fixed_length, and__mod__should be used as they are evaluated in linear time (also the results are cached).If PyDSDL is asked to perform a slow numerical expansion, the stack trace of the request is logged to help one hunt down dependencies on the old API.
I am going to update the Nunavut templates soon. While doing that, I am likely to find deficiencies of the new implementation
so the version is set to.devfor nowDSDL expressions evaluated for
@assert,@print,@sealed,[...]etc always trigger numerical expansion for now so one should be careful with these. It is possible to update the expression evaluation logic to take advantage of the solver where possible but this has not been done yet and it is not on my roadmap because it doesn't seem important. I did, however, put a TODO comment or two.This is how you can install it for testing locally: