-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Specify by Attributes #63
base: master
Are you sure you want to change the base?
Conversation
Rename Test/launch_test.sh to reflect that it does *not* run unittests. Add Test/README.md for clarification. Apply workaround to make `import picmistandard` work in tests using a symlink -- in the future maybe mv source to (more canonical) /lib/python/picmistandard ?
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 |
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 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. |
The format before this commit has been proposed in PEP 224, which has been rejected. Aim of this format is to avoid duplicating the list of attributes in the class body and class docstring. However, the old format is only understood by some tools, so we return to plain PEP 257 docstrings. Note: An alternative would be to add a __doc_ATTRNAME__ string, which would only exist as convention. (Could be enforced inside PICMI though.)
Thanks - to me, the pep257 style doc strings is cleaner. BTW, does this need |
@@ -0,0 +1,28 @@ | |||
# Testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding unit tests. Definitely something this repo needs.
For discussion on Monday:
|
This reverts commit e5000b1. The case it tried to cover might bite our back down the road, but it is very non-pythonic (difficult, dirty) and does not perform well once methods get involved, so dropped it.
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. |
Note to self: add documentation how to perform computations based on dataclasses |
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. |
@@ -1,2 +1,3 @@ | |||
numpy~=1.15 | |||
scipy~=1.5 | |||
typeguard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked: pure python (incl. dependencies) & easy to install it seems
""" | ||
checks self, raises on error, passes silently if okay | ||
|
||
! MUST NOT BE OVERWRITTEN ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: hard to enforce API contract
|
||
See class docstring for detailed description. | ||
|
||
! MUST NOT BE OVERWRITTEN ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: hard to enforce, maybe even error-prune API contract and maybe not ideal for something as common as __init__
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++) Proposed way forward:
|
s910597 has most likely moved one |
@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 |
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):
check()
method. If found, execute in constructor.Arguable design decisions:
__init__()
can still be overwrittentyping.Any
are forbidden (b/c there are no typechecks enforced)warpx_bla
in fbpic) is allowed and passes silently