♻️ Don't use None as default for required arguments

[s-weigand]

• glotaran version: 0a3f5e2
• Python version: 3.8.6
• Operating System: Win64

Description

While writing type stub files for the runtime patched objects (Models and Modelattributes) due to known limitations of mypy. I found that some functions have default value of None for arguments that are required. This suggests that you could use them without or fewer than required arguments.
Also, to make sense of the tracebacks the user needs detailed knowledge of the glotaran internals e.g. the model decorator.

What I Did

The signature of the model decorator

pyglotaran/glotaran/model/decorator.py

Lines 59 to 76 in 0a3f5e2

	def model(
	model_type: str,
	attributes: Dict[str, Any] = None,
	dataset_type: Type[DatasetDescriptor] = DatasetDescriptor,
	megacomplex_type: Any = None,
	matrix: Union[MatrixFunction, IndexDependedMatrixFunction] = None,
	global_matrix: GlobalMatrixFunction = None,
	model_dimension: str = None,
	global_dimension: str = None,
	has_matrix_constraints_function: Callable[[Type[Model]], bool] = None,
	constrain_matrix_function: ConstrainMatrixFunction = None,
	retrieve_clp_function: RetrieveClpFunction = None,
	has_additional_penalty_function: Callable[[Type[Model]], bool] = None,
	additional_penalty_function: PenaltyFunction = None,
	finalize_data_function: FinalizeFunction = None,
	grouped: Union[bool, Callable[[Type[Model]], bool]] = False,
	index_dependent: Union[bool, Callable[[Type[Model]], bool]] = False,
	) -> Callable:

suggests that you could use it like this to generate the most minimal (even so nonsensical) Model

@model("foo")
class FooModel:
    pass

While the most minimal way for it not to crash is

class DummyAttr:
    _glotaran_has_label = False

@model("foo", attributes={}, megacomplex_type=DummyAttr, matrix=lambda: None)
class FooModel:
    pass

Another instance where this is the case is in kinetic_image_matrix

pyglotaran/glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py

Lines 11 to 19 in 0a3f5e2

	def kinetic_image_matrix(dataset_descriptor=None, axis=None, index=None, irf=None):
	return kinetic_matrix(
	dataset_descriptor, axis, index, irf, kinetic_image_matrix_implementation
	)
	def kinetic_matrix(
	dataset_descriptor=None, axis=None, index=None, irf=None, matrix_implementation=None
	):

Where nearly all arguments are required but defaulted to None.

[s-weigand]

When talking to @joernweissenborn he explained that the idea behind None default values is to make them keyword arguments instead of positional ones.

A quick reminder on how 'normal' arguments work in python. They allow both the usage as keyword arguments AND positional arguments.

In [1]: def f(x, y=1):
      :     return x + y
      :

In [2]: f(2)
Out[2]: 3

In [3]: f(x=2)
Out[3]: 3

In [4]: f(y=3, x=2)
Out[4]: 5

In [5]: f(1, 2)
Out[5]: 3

To force users to only use keyword arguments, for better readability (more verbose and unambiguous) e.g. for the model decorator this would make sense, we could use keyword only arguments

In [7]: g(2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-7-3580407364ae> in <module>
----> 1 g(2)

TypeError: g() takes 0 positional arguments but 1 was given

In [8]: g(y=3, x=2)
Out[8]: 5

If we want users to only use positional arguments (the only reasonable use case, being not yet final argument names) we could use positional only arguments, since our new min version of python is 3.8

In [2]: def h(x, y=1, /):
   ...:     return x + y
   ...:

In [3]: h(x=2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-ad2c09c39c80> in <module>
----> 1 h(x=2)

TypeError: h() got some positional-only arguments passed as keyword arguments: 'x'

In [4]: h(2)
Out[4]: 3

In [5]: h(2, 3)
Out[5]: 5

Please note that those can also be mixed.

Here is also a nice video explaining how they work (just as a reference) 😉

[s-weigand]

Just a Note (rev: f366ed6):
By now a minimal dummy model implementation (in my case for testing the plugin system) needs to look like this, to not throw an error when creating.

class DummyAttr:
    _glotaran_has_label = False

@model(
    "foo",
    attributes={},
    megacomplex_type=DummyAttr,
    matrix=lambda: None,  # type:ignore
    model_dimension="",
    global_dimension="",
)
class FooModel:
    pass

pyglotaran/glotaran/model/decorator.py

Lines 78 to 95 in f366ed6

	def model(
	model_type: str,
	attributes: dict[str, Any] = None,
	dataset_type: type[DatasetDescriptor] = DatasetDescriptor,
	megacomplex_type: Any = None,
	matrix: MatrixFunction | IndexDependentMatrixFunction = None,
	global_matrix: GlobalMatrixFunction = None,
	model_dimension: str = None,
	global_dimension: str = None,
	has_matrix_constraints_function: Callable[[type[Model]], bool] = None,
	constrain_matrix_function: ConstrainMatrixFunction = None,
	retrieve_clp_function: RetrieveClpFunction = None,
	has_additional_penalty_function: Callable[[type[Model]], bool] = None,
	additional_penalty_function: PenaltyFunction = None,
	finalize_data_function: FinalizeFunction = None,
	grouped: bool | Callable[[type[Model]], bool] = False,
	index_dependent: bool | Callable[[type[Model]], bool] = False,
	) -> Callable[[type[Model]], type[Model]]:

[jsnel]

I think a lot of improvement toward this issue were incorporated in #1135, perhaps time for another review.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been closed due to lack of recent activity. Please open a new issue if you're still encountering this problem. Thanks for your contributions!