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] Added test of type checking and autoargs #71

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
85 changes: 34 additions & 51 deletions PICMI_Python/applied_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,20 @@
These should be the base classes for Python implementation of the PICMI standard
"""
import re
import typing
from collections.abc import Sequence

from .base import _ClassWithInit
from autoclass import autoargs
from typeguard import typechecked

from .base import _ClassWithInit, VectorFloat3, Expression

# ---------------
# Applied fields
# ---------------


@typechecked
class PICMI_ConstantAppliedField(_ClassWithInit):
"""
Describes a constant applied field
Expand All @@ -22,23 +28,21 @@ class PICMI_ConstantAppliedField(_ClassWithInit):
- lower_bound=[None,None,None]: Lower bound of the region where the field is applied (vector) [m]
- upper_bound=[None,None,None]: Upper bound of the region where the field is applied (vector) [m]
"""
def __init__(self, Ex=None, Ey=None, Ez=None, Bx=None, By=None, Bz=None,
lower_bound=[None,None,None], upper_bound=[None,None,None],
**kw):

self.Ex = Ex
self.Ey = Ey
self.Ez = Ez
self.Bx = Bx
self.By = By
self.Bz = Bz

self.lower_bound = lower_bound
self.upper_bound = upper_bound
@autoargs(exclude=['kw'])
def __init__(self, Ex : float = None,
Ey : float = None,
Ez : float = None,
Bx : float = None,
By : float = None,
Bz : float = None,
lower_bound : VectorFloat3 = [None,None,None],
upper_bound : VectorFloat3 = [None,None,None],
**kw):

self.handle_init(kw)


@typechecked
class PICMI_AnalyticAppliedField(_ClassWithInit):
"""
Describes an analytic applied field
Expand All @@ -54,35 +58,16 @@ class PICMI_AnalyticAppliedField(_ClassWithInit):
- lower_bound=[None,None,None]: Lower bound of the region where the field is applied (vector) [m]
- upper_bound=[None,None,None]: Upper bound of the region where the field is applied (vector) [m]
"""
def __init__(self, Ex_expression=None, Ey_expression=None, Ez_expression=None,
Bx_expression=None, By_expression=None, Bz_expression=None,
lower_bound=[None,None,None], upper_bound=[None,None,None],
**kw):

self.Ex_expression = Ex_expression
self.Ey_expression = Ey_expression
self.Ez_expression = Ez_expression
self.Bx_expression = Bx_expression
self.By_expression = By_expression
self.Bz_expression = Bz_expression

self.lower_bound = lower_bound
self.upper_bound = upper_bound

# --- Find any user defined keywords in the kw dictionary.
# --- Save them and delete them from kw.
# --- It's up to the code to make sure that all parameters
# --- used in the expression are defined.
self.user_defined_kw = {}
for k in list(kw.keys()):
if ((self.Ex_expression is not None and re.search(r'\b%s\b'%k, self.Ex_expression)) or
(self.Ey_expression is not None and re.search(r'\b%s\b'%k, self.Ey_expression)) or
(self.Ez_expression is not None and re.search(r'\b%s\b'%k, self.Ez_expression)) or
(self.Bx_expression is not None and re.search(r'\b%s\b'%k, self.Bx_expression)) or
(self.By_expression is not None and re.search(r'\b%s\b'%k, self.By_expression)) or
(self.Bz_expression is not None and re.search(r'\b%s\b'%k, self.Bz_expression))):
self.user_defined_kw[k] = kw[k]
del kw[k]
@autoargs(exclude=['kw'])
def __init__(self, Ex_expression : Expression = None,
Ey_expression : Expression = None,
Ez_expression : Expression = None,
Bx_expression : Expression = None,
By_expression : Expression = None,
Bz_expression : Expression = None,
lower_bound : VectorFloat3 = [None,None,None],
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

upper_bound : VectorFloat3 = [None,None,None],
**kw):

self.handle_init(kw)

Expand All @@ -103,17 +88,15 @@ class PICMI_Mirror(_ClassWithInit):
or the code's default value if neither are specified.
"""

def __init__(self, x_front_location=None, y_front_location=None, z_front_location=None,
depth=None, number_of_cells=None, **kw):
def __init__(self, x_front_location : float = None,
y_front_location : float = None,
z_front_location : float = None,
depth : float = None,
number_of_cells : int = None,
**kw):

assert [x_front_location,y_front_location,z_front_location].count(None) == 2,\
Exception('At least one and only one of [x,y,z]_front_location should be specified.')

self.x_front_location = x_front_location
self.y_front_location = y_front_location
self.z_front_location = z_front_location
self.depth = depth
self.number_of_cells = number_of_cells

self.handle_init(kw)

33 changes: 33 additions & 0 deletions PICMI_Python/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
"""
import inspect
import warnings
import typing
from collections.abc import Sequence
import re


codename = None

Expand All @@ -26,8 +30,37 @@ def register_constants(implementation_constants):
def _get_constants():
return _implementation_constants


VectorFloat3 = typing.NewType('VectorFloat3', Sequence[float])
VectorInt3 = typing.NewType('VectorInt3', Sequence[int])
Expression = typing.NewType('Expression', str)

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

But ultimately your call.

Copy link
Member Author

Choose a reason for hiding this comment

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

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



class _ClassWithInit(object):
def _check_vector_lengths(self):
for arg_name, arg_type in self.__init__.__annotations__.items():
if arg_type in [VectorFloat3, VectorInt3]:
arg_value = getattr(self, arg_name)
assert len(arg_value) == 3, Exception(f'{arg_name} must have a length of 3')

def _process_expression_arguments(self, kw):
"""For arguments that are of type Expression, save any keyword arguments used in
the expression in the user_defined_kw dictionary.
"""
for arg_name, arg_type in self.__init__.__annotations__.items():
if arg_type == Expression:
# Create the dictionary is needed
self.user_defined_kw = getattr(self, 'user_defined_kw', {})
arg_value = getattr(self, arg_name)
if arg_value is not None:
for k in list(kw.keys()):
if re.search(r'\b%s\b'%k, arg_value):
self.user_defined_kw[k] = kw.pop(k)

def handle_init(self, kw):
self._check_vector_lengths()
self._process_expression_arguments(kw)

# --- Grab all keywords for the current code.
# --- Arguments for other supported codes are ignored.
# --- If there is anything left over, it is an error.
Expand Down
2 changes: 2 additions & 0 deletions PICMI_Python/requirements.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
numpy~=1.15
scipy~=1.5
autoclass
Copy link
Member

Choose a reason for hiding this comment

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

typeguard
Copy link
Member

Choose a reason for hiding this comment

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