Skip to content
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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

s9105947
Copy link

@s9105947 s9105947 commented Mar 2, 2022

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

  • semantic checks
    • look for override-able check() method. If found, execute in constructor.
  • custom vars for *analytic objects
    • add flag that can be set to enable custom vars
    • note: the syntax for the given equations is not defined, maybe define this first?
  • breaks compatibility to [WIP]Handle unsupported #61
    • merge & adjust here
  • offer shortcuts to numeric types, lists, (2D or 3D vectors?)
  • provide documentation
  • doc: add "howto calculations for dataclasses"

Arguable design decisions:

  • __init__() can still be overwritten
  • type annotations if not typing.Any are forbidden (b/c there are no typechecks enforced)
  • using a codespecific attribute if not using that code (e.g. using warpx_bla in fbpic) is allowed and passes silently

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 ?
@s9105947 s9105947 changed the title Rework dataclass [WIP] Specify by Attributes Mar 2, 2022
@s9105947
Copy link
Author

s9105947 commented Mar 7, 2022

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
Copy link
Member

dpgrote commented Mar 9, 2022

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.

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.)
@dpgrote
Copy link
Member

dpgrote commented Mar 11, 2022

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

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

@@ -0,0 +1,28 @@
# Testing
Copy link
Member

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.

@ax3l
Copy link
Member

ax3l commented Mar 11, 2022

For discussion on Monday:

Test/unit/base.py Outdated Show resolved Hide resolved
@s9105947
Copy link
Author

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
Copy link
Author

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

@dpgrote
Copy link
Member

dpgrote commented Mar 16, 2022

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
checks self, raises on error, passes silently if okay

! MUST NOT BE OVERWRITTEN !
Copy link
Member

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 !
Copy link
Member

@ax3l ax3l Mar 16, 2022

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__

@s9105947
Copy link
Author

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:

  • Use PR [WIP] Added test of type checking and autoargs #71 to supply checks & cut boilerplate.
  • Carry Tests & Documentation from this PR over into a new one (and adjust them).
  • We discussed a general PICMI design document to clarify the misunderstanding outlined above (and more), PR will follow.

@ax3l
Copy link
Member

ax3l commented May 1, 2023

@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
Copy link
Member

s910597 has most likely moved one

@BrianMarre
Copy link
Member

BrianMarre commented May 4, 2023

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants