[WIP] Added test of type checking and autoargs

[dpgrote]

This is an alternative proposal to what PR #63 is doing. This set of changes is much smaller but still does all of the things desired.

• Reduce the number of times parameters are referenced
• Runtime type checking on scalars and vectors
• Checks lengths of scalars
• Handles keyword arguments as before (allowing codename_arg with checking)
• Fully backwards compatible (and allows class to be updated one by one)
• Allows argument value checking in the __init__ routine or separate callable method
• The type checking has been added to all classes

Units tests need to be added, but there isn't much to test since it is using commonly used packages.
Documentation can be added (much of that from PR #63 can be carried over).

[ax3l]

Checked: pure python (incl. dependencies) & easy to install it seems

• https://pypi.org/project/autoclass/#files
• https://github.com/smarie/python-autoclass/blob/master/pyproject.toml
• https://github.com/smarie/python-autoclass/blob/master/setup.py

[ax3l]

Checked: pure python (incl. dependencies) & easy to install it seems

• https://pypi.org/project/typeguard/#files
• https://github.com/agronholm/typeguard/blob/master/pyproject.toml
• https://github.com/agronholm/typeguard/blob/master/setup.cfg

[ax3l]

Is assigning a list as default argument value (old and new) actually possible?

Common Python bug:
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

[dpgrote]

Yes, this code works as is, and I am aware of the issue. In this case, however, it is not expected that the argument, lower_bound, will be modified, but only read. An alternative here would be to have lower_bound default to None. This would be more consistent with other vectors that default to None. In this PR, I wanted to avoid changing the interface, but the change can be made.

[s9105947]

I like the short form very much, this also gets very close to the python dataclass notation which performs almost the same by specifying class attributes.

The following details raised my attention:

• Do we need developer-overwriteable __init__() methods? I am worried about the possibility to simply specify arbitrary code -- this would undermine the predictability of PICMI objects.
• How are mandatory attributes treated?
• What is the difference between a default argument of the wrong type, and a user-supplied argument of the default type.
• How is this interface used to extend existing PICMI classes in PICMI implementations?

These are design principles, I've sent you an email for a short meeting on the matter to put these design principles into writing.

[dpgrote]

Thanks for looking at this PR. Here are comments on the issues:

• Regarding __init__, this is where any checks can be done. I don't think there is an issue with adding code to the __init__ routine (at the picmi level). Remember that picmi is monitored and that any changes would need to be approved. For the implementations, the standard would be that the implementation not overwrite the __init__ routine but only add the init method to do what is needed.
• Mandatory arguments are setup the same way but just without a default value, e.g. n_physical_particles : int, .
• I checked this. The default argument does not need to have the specified type (so None is ok). If the user sets the argument to the default value, it is allowed. This is ok, since there is no way to tell the difference.
• The changes here don't require any changes to the implementation (from what it was before).

[ax3l]

That sounds great to me.

I would probably still introduce a check member function as we have also the init member function.
That way, we can check physical ranges both in the constructor (or init) as well as before we call the step() or inputs export, respectively. And thus, we can check parameters that users changed in between, e.g., though custom setters, their own properties, or accessing a member variable directly after construction.

[dpgrote]

This check method can be added, but I would suggest that this should be done in a separate PR.

Once the changes here are accepted, I would suggest that this PR be merged. The other classes can be updated to use the new features in separate PRs (to keep the PRs small). This also allows the work to be spread out (since people are busy).

[s9105947]

How do you fell about moving this into a separate namespace (for python a module i.e. here a file)?

This way every type will get a prefix and the distinction between python native & PICMI custom (and perhaps other types) will become clear.

[dpgrote]

Yes, I was considering that, so that all of the type declarations are in the same place. It is somewhat tricky though because of the circular imports that will happen since some of the types depend on PICMI classes which will depend on the types. I'll see what I can put together.

[s9105947]

had that problem too, PEP 484 allows using strings instead of literals for forward declarations

But ultimately your call.

[dpgrote]

Ok, thanks, using strings worked. I am clearly not the first person to come across this circular definition problem :)

[s9105947]

same as above: How do you feel about collecting all type definitions in a single file?

I am worried that existing type definitions might be overlooked and defined over and over again.

[dpgrote]

I've implemented the type checking in all of the classes. All of the defined types are not setup in the picmi_types.py files. Though, there is some kludginess there because of the way the type definitions that are in strings are evaluated. For example, for each type that is used, a module must import the modules used in the type definition (that's why particles.py must import fields for instance).

The problem comes when a type is used that depends on the same module where the type is used. In this case, the module can't import itself, so the type string cannot reference the module. For this reason, I had to define two versions of the GridType, with and without the fields references

An alternative is to do "*" imports, i.e. particles could do "from .fields import *" (so the fields prefix in the type definition is not needed), but this has its own messiness. I'm receptive to other alternative ideas.