liesel-devs / liesel

A probabilistic programming framework
https://liesel-project.org
MIT License
38 stars 2 forks source link

Var constructors: new_param, new_obs, new_calc, new_value #201

Open jobrachem opened 1 month ago

jobrachem commented 1 month ago

For a while now, I have been a little unsatisfied with two things:

  1. Incsonsistent UI: Var class vs. param and obs functions. See also #130
  2. The status of the Data (or, in #170 : Value) node vs. Var: Which should you use?

I think there is a satisfying solution:

  1. Implement constructors for Var that fullfill the tasks above.
  2. Guide users to towards using Var.new_<constructor> as a default.

If I recall correctly, using constructors has been proposed before by @wiep.

This PR implements four constructors.

Var.new_param

Supersedes lsl.param()

class Var:

    @classmethod
    def new_param(
        cls, value: Any, distribution: Dist | None = None, name: str = ""
    ) -> Self:
        ...

Var.new_obs

Supersedes lsl.obs()

class Var:
    @classmethod
    def new_obs(
        cls, value: Any, distribution: Dist | None = None, name: str = ""
    ) -> Self:
        ...

Var.new_calc

New. Having this constructor available makes the interface more consistent. Users can simply start typing Var.new_ and see the available options via auto-completion. It also reduces errors from confusion about nesting Calc objects in Var constructors.

class Var:
    @classmethod
    def new_calc(
        cls,
        function: Callable[..., Any],
        *inputs: Any,
        name: str = "",
        _needs_seed: bool = False,
        update_on_init: bool = True,
        **kwinputs: Any,
    ) -> Self:
        ...

Var.new_const

New. Like with Var.new_calc, having this constructor makes the interface more consistent and supports a good habit.

class Var:
    @classmethod
    def new_const(cls, value: Any, name: str = "") -> Self:
        ...

Todo

This is a draft PR for a first discussion. Even if it remains unchanged, documentation has to be added if it ends up being merged.

jobrachem commented 2 weeks ago

Discussion notes

What is the purpose of .new_const?

Maybe .new_const just needs a better name. Maybe lsl.Var.new_value? That would provide a nice parallel to how lsl.Var.new_calc wraps a lsl.Calc.

jobrachem commented 6 days ago

This PR is now ready for review. In addition to th new constructors, I added and updated the documentation:

The helper functions lsl.param and lsl.obs are marked as deprecated and emit a FutureWarning now. They are announced as scheduled for removal in v0.4.0. Currently we are moving towards v0.2.10.

During the review, please pay attention not only to the pure code implementation, but also to the clarity of the documentation 🙏