picmi-standard / picmi

Standard input format for Particle-In-Cell codes
https://picmi.readthedocs.io
Other
35 stars 25 forks source link

[WIP] Added test of type checking and autoargs #71

Open dpgrote opened 2 years ago

dpgrote commented 2 years ago

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.

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).

s9105947 commented 2 years ago

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:

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 commented 2 years ago

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

ax3l commented 2 years ago

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 commented 2 years ago

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).

dpgrote commented 2 years ago

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.