picmi-standard / picmi

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

[WIP] Specify by Attributes #63

Open s9105947 opened 2 years ago

s9105947 commented 2 years ago

Demonstrates intended transformation as described in #60.

See PICMI_Python/applied_fields.py as an example usage, Test/unit/base.py for tests.

Not yet addressed issues (+solution):

Arguable design decisions:

s9105947 commented 2 years ago

Before I go down the road of applying this to the entire codebase & implementing the open tasks listed above:

@dpgrote, if you could please have a look at the transformed definition of PICMI_ConstantAppliedField, is this a concept you can get behind? Should it be applied to the remaining standard?

@RemiLehe @ax3l

dpgrote commented 2 years ago

Sorry, but I am not enthusiastic about these changes. Overall, I find the code harder to read, with the list of input parameters spread out and not easy to visually scan (when looking something up for example). Does Sphinx/Doxygen understand this format when generating documentation? Also, the code for checking arguments is more complicated but doesn't seem to do anything more than what is already there, in handle_init method.

I understand the desire to have more type checking of the input parameters, but I don't think that the changes in this PR are the right way to go about it.

dpgrote commented 2 years ago

Thanks - to me, the pep257 style doc strings is cleaner.

BTW, does this need @autoclass before the class is defined?

ax3l commented 2 years ago

For discussion on Monday:

s9105947 commented 2 years ago

This is roughly how I imagined the draft to look, could you please have a look and give me some feedback?

I have a way to handle custom variables in free expressions sketched on my whiteboard, will implement tomorrow (it does only require very little changes).

If we can agree on this draft I will start porting a first file entirely to this dataclass approach.

s9105947 commented 2 years ago

Note to self: add documentation how to perform computations based on dataclasses

dpgrote commented 2 years ago

I've added PR #71 as a counter proposal to this PR. The changes made there are much smaller and lighter weight. Please take a look at it.

s9105947 commented 2 years ago

Discussed this concept with @dpgrote to a some depth.

I was under the false impression that PICMI provides a pure data structure (schema), and this PR makes a pure schema conveniently usable from Python. (Pretty much dataclass++) This is not the case, PICMI is primarily an interface, hence the existence methods is not a problem.

Proposed way forward:

ax3l commented 1 year ago

@dpgrote as discussed recently, I think dataclasses could be the way to go, but going one step further and using pydantic #94 for validators and export of the API to other formats, e.g., toml/yaml/xml.

@s9105947 any opinion on #94?

BrianMarre commented 1 year ago

s910597 has most likely moved one

BrianMarre commented 1 year ago

@ax3l I quite like the pydantic approach, it seems to creates less boiler plate code than the pure typing approach we are using in the PIConGPU PICMI implementation.

And in the end I do not care what we use, so long as we get typing/type-checking