Conversation
wrdxwrdxwrdx
force-pushed
the
distribution/sampling
branch
2 times, most recently
from
a1de7e9 to
d8ff3f0
Compare
LeonidElkin
left a comment
LeonidElkin
left a comment
There was a problem hiding this comment.
Reviewed only core/* except for _unuran_sampler
| @@ -9,10 +8,9 @@ jobs: | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| os: [ ubuntu-latest, macos-latest, windows-latest ] | |||
| python-version: [ "3.12", "3.13" ] | |||
| os: [ubuntu-latest, macos-latest, windows-latest] | |||
| python-version: ["3.12", "3.13"] | |||
| runs-on: ${{ matrix.os }} | |||
|
|
|||
There was a problem hiding this comment.
Probably some tools could auto-format it. Such diffs shouldn't be in the PR
examples/plots/discrete_dgt.png
Outdated
There was a problem hiding this comment.
I think this and other pictures could be inside of jupiter notebook using matplotlib. There is no need to store it directly in our repository
| "nbconvert_exporter": "python", | ||
| "pygments_lexer": "ipython3", | ||
| "version": "3.12.3" | ||
| } |
There was a problem hiding this comment.
You probably execute this notebook with other kernel version or something like that so it rewrites the metadata. You definitely shouldn't have committed this diff, because it doesn't affect the repository in any way
| _unuran_cffi_module: ModuleType | None = None | ||
| try: | ||
| _unuran_cffi_module = import_module("pysatl_core.sampling.unuran.bindings._unuran_cffi") | ||
| except ModuleNotFoundError: # pragma: no cover - optional binary module | ||
| try: | ||
| _unuran_cffi_module = import_module("_unuran_cffi") | ||
| except ModuleNotFoundError: # pragma: no cover - optional binary module | ||
| _unuran_cffi_module = None |
There was a problem hiding this comment.
You could use something like that to avoid unnecessary nesting. And it seems more natural I think
_unuran_cffi_module = None
for name in (
"pysatl_core.sampling.unuran.bindings._unuran_cffi",
"_unuran_cffi",
):
try:
_unuran_cffi_module = import_module(name)
break
except ModuleNotFoundError: # pragma: no cover - optional binary module
pass| if _unuran_cffi is None: | ||
| raise RuntimeError( | ||
| "UNURAN CFFI bindings are not available. " | ||
| "Please build them via `python " | ||
| "poetry build" |
There was a problem hiding this comment.
The message just ends. Adjust it a little
| When True, the last sampler and distribution are cached and reused | ||
| if the same distribution object is used in subsequent calls. | ||
| """ | ||
| self._default_config = default_config or UnuranMethodConfig() # TODO config not default |
There was a problem hiding this comment.
What do you mean "default config" . Shouldn't we just pass the config and if it wasn't passed, use the default value, that is just UnuranMethodConfig()
There was a problem hiding this comment.
After looking at this file in detail, I come to the conclusion that what we call a strategy is not actually a strategy. In fact, the same sampler can be called just a strategy and it will make more sense. It seems to me that this is an abstraction for demolition, that is, this file and, in general, such an abstraction is not needed.
| 6. Initializes the UNURAN generator | ||
| """ | ||
| self.distr = distr | ||
| self.config = config or UnuranMethodConfig() |
There was a problem hiding this comment.
If it means that the config can be changed in runtime and everything will work normally, then ok. If not, it is better to do it as a readonly, through the property
| def _cleanup(self) -> None: | ||
| cleanup_unuran_resources(self) |
There was a problem hiding this comment.
I think it's better to do it honestly with a class method.
There was a problem hiding this comment.
I think it's better to do it as a static DefaultUnuranSampler method.
LeonidElkin
added
API: Design
ALG: Sampling
LeonidElkin
left a comment
LeonidElkin
left a comment
There was a problem hiding this comment.
Implement tests as packages with __init__.py
| f"Unsupported distribution type: {distr_type}. " | ||
| "Only Euclidean distribution types are supported." | ||
| ) | ||
| self._is_continuous = distr_type.kind == Kind.CONTINUOUS |
There was a problem hiding this comment.
Let's store it as a kind, not as a bool
| if self._is_continuous: | ||
| self._unuran_distr = self._lib.unur_distr_cont_new() | ||
| else: | ||
| self._unuran_distr = self._lib.unur_distr_discr_new() |
There was a problem hiding this comment.
Replace it with a switch case
| if hasattr(self, "_unuran_gen") and self._unuran_gen is not None: | ||
| if self._unuran_gen != self._ffi.NULL: | ||
| with contextlib.suppress(Exception): | ||
| self._lib.unur_free(self._unuran_gen) | ||
| gen_freed = True | ||
| self._unuran_gen = None | ||
|
|
||
| if hasattr(self, "_unuran_par") and self._unuran_par is not None: | ||
| if not gen_freed and self._unuran_par != self._ffi.NULL: | ||
| with contextlib.suppress(Exception): | ||
| self._lib.unur_par_free(self._unuran_par) | ||
| self._unuran_par = None | ||
|
|
||
| if hasattr(self, "_unuran_distr") and self._unuran_distr is not None: | ||
| if self._unuran_distr != self._ffi.NULL: | ||
| with contextlib.suppress(Exception): | ||
| self._lib.unur_distr_free(self._unuran_distr) | ||
| self._unuran_distr = None |
There was a problem hiding this comment.
No need in hasattr if you initialize it as None and it is not a classmethod nor staticmethod
| for characteristic in available_chars: | ||
| if characteristic in self._distr.analytical_computations: | ||
| distr_characteristic[characteristic] = cast( | ||
| "Method[Any, Any]", self._distr.analytical_computations[characteristic] | ||
| ) | ||
| else: | ||
| distr_characteristic[characteristic] = self._distr.query_method(characteristic) |
There was a problem hiding this comment.
query_method is already checking that characteristic is analytical
| characteristics: Mapping[CharacteristicName, Method[Any, Any]], | ||
| ): | ||
| self._unuran_distr = unuran_distr | ||
| self._is_continuous = is_continuous |
There was a problem hiding this comment.
Change with Kind, not bool
| def determine_domain_from_support(self) -> tuple[int, int | None] | None: | ||
| """ | ||
| Determine domain boundaries from distribution support if available. | ||
|
|
||
| Returns | ||
| ------- | ||
| tuple[int, int | None] or None | ||
| Domain as (left, right) if support is bounded, or (left, None) if only | ||
| left boundary is known, or None if support is unavailable/unbounded. | ||
| """ | ||
| support = self._distr.support | ||
| if support is None: | ||
| return None | ||
|
|
||
| if isinstance(support, ExplicitTableDiscreteSupport): | ||
| points = support.points | ||
| if points.size == 0: | ||
| return None | ||
| left = int(np.floor(points[0])) | ||
| right = int(np.ceil(points[-1])) | ||
| return (left, right) | ||
|
|
||
| if isinstance(support, IntegerLatticeDiscreteSupport): | ||
| first = support.first() | ||
| last = support.last() | ||
|
|
||
| if first is not None and last is not None: | ||
| return (first, last) | ||
|
|
||
| if first is not None: | ||
| return (first, None) | ||
|
|
||
| return None | ||
|
|
||
| # Fallback: try to call first() and last() methods | ||
| # if not IntegerLatticeDiscreteSupport or ExplicitTableDiscreteSupport | ||
| first_callable = getattr(support, "first", None) | ||
| last_callable = getattr(support, "last", None) | ||
|
|
||
| if callable(first_callable) and callable(last_callable): | ||
| try: | ||
| first = first_callable() | ||
| last = last_callable() | ||
| if first is not None and last is not None: | ||
| return (int(first), int(last)) | ||
| except TypeError: | ||
| pass | ||
|
|
||
| return None | ||
|
|
||
| def _get_continuous_support_bounds(self) -> tuple[float, float] | None: | ||
| support = getattr(self._distr, "support", None) | ||
| if support is None: | ||
| return None | ||
|
|
||
| left = getattr(support, "left", None) | ||
| right = getattr(support, "right", None) | ||
|
|
||
| if isinstance(left, int | float) and isinstance(right, int | float): | ||
| return float(left), float(right) | ||
|
|
||
| # Fallback to callable accessors if available (e.g., support.first()/last()) | ||
| left_fn = getattr(support, "first", None) | ||
| right_fn = getattr(support, "last", None) | ||
| try: | ||
| left_val = float(left_fn()) if callable(left_fn) else None | ||
| right_val = float(right_fn()) if callable(right_fn) else None | ||
| except TypeError: | ||
| left_val = right_val = None | ||
|
|
||
| if left_val is not None and right_val is not None: | ||
| return left_val, right_val | ||
|
|
||
| return None |
There was a problem hiding this comment.
Let's name these functions in the same way (smth like determine_discrete_domain and determine_continuous_domain) and make them both public, not private
| def __init__(self, disr: Distribution) -> None: | ||
| self._distr = disr |
| if isinstance(support, ExplicitTableDiscreteSupport): | ||
| points = support.points | ||
| if points.size == 0: | ||
| return None | ||
| left = int(np.floor(points[0])) | ||
| right = int(np.ceil(points[-1])) | ||
| return (left, right) |
There was a problem hiding this comment.
There is need in fallback to our default sampler if there is even one non int number in the table
| # Fallback: try to call first() and last() methods | ||
| # if not IntegerLatticeDiscreteSupport or ExplicitTableDiscreteSupport | ||
| first_callable = getattr(support, "first", None) | ||
| last_callable = getattr(support, "last", None) | ||
|
|
||
| if callable(first_callable) and callable(last_callable): | ||
| try: | ||
| first = first_callable() | ||
| last = last_callable() | ||
| if first is not None and last is not None: | ||
| return (int(first), int(last)) | ||
| except TypeError: | ||
| pass |
| return None | ||
| left = int(np.floor(points[0])) | ||
| right = int(np.ceil(points[-1])) | ||
| return (left, right) |
There was a problem hiding this comment.
Remove here and in other places redundant parentheses
|
Rename |
wrdxwrdxwrdx
force-pushed
the
distribution/sampling
branch
from
81c9dc0 to
1478930
Compare
wrdxwrdxwrdx
force-pushed
the
distribution/sampling
branch
from
1478930 to
cd12314
Compare
wrdxwrdxwrdx
force-pushed
the
distribution/sampling
branch
from
cd12314 to
5c22a36
Compare
| _CDEF_FILE = Path(__file__).with_name("cffi_unuran.h") | ||
| UNURAN_CDEF: Final = _CDEF_FILE.read_text() | ||
|
|
||
| LOGGER = logging.getLogger(__name__) |
There was a problem hiding this comment.
Where can I see the log if something goes wrong?
| if self._cleaned_up: | ||
| return | ||
|
|
||
| self._cleaned_up = True |
There was a problem hiding this comment.
I think it's better to move it to the bottom of the method for safety
| @property | ||
| def _config(self) -> UnuranMethodConfig: | ||
| """The configuration for this sampler (read-only).""" | ||
| return self.__config |
There was a problem hiding this comment.
I think we could actually remove this method. Strategy is already giving us a way to reach config. This code isn't used anywhere else
There was a problem hiding this comment.
And also could make self.__config just self._config
| UnuranMethodConfig, | ||
| ) | ||
|
|
||
| from .unuran_sampler import DefaultUnuranSampler |
| def __init__( | ||
| self, | ||
| distr: Distribution, | ||
| config: UnuranMethodConfig | None = None, | ||
| **override_options: Any, | ||
| ) -> None: | ||
| """Initialize the sampler for ``distr`` with optional configuration overrides.""" | ||
| ... |
There was a problem hiding this comment.
There is no need in __init__ in Protocol. Correct me if I'm wrong.
There was a problem hiding this comment.
Add to every __init__.py in tests such code
"""
PySATL Core
===========
Core framework for probabilistic distributions providing type definitions,
distribution abstractions, characteristic computation graphs, and parametric
family management.
"""
__author__ = "YOUR NAME"
__copyright__ = "Copyright (c) 2025 PySATL project"
__license__ = "SPDX-License-Identifier: MIT"2 participants
UNU.RAN integration
Start with notebooks and scripts under
examples/—they showcase end-to-end scenarios for the new sampling strategy.Now you need to run this after git clone
git submodule update --init --remote --recursiveNow unuran submodule cloning from dev branch. Please review PR
WARNING
Added dpdf to registry. This is done due to the fact that many methods require it to work. In the future, it's worth implementing it more correctly in the registry.
Platforms
Added
Characteristics & methods