-
Notifications
You must be signed in to change notification settings - Fork 17
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
form and const formatting #550
Conversation
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.
Looks good! See comments for the next round.
def sort_vector(fml: str, symbs: list[str] | None = None): | ||
def sort_vector( | ||
fml: Sequence[str], symbs: Sequence[str] | None = None | ||
) -> tuple[int, ...]: |
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.
fml
should be Sequence[Formula]
.
NameRow = tuple[Name, Name, Name] | ||
KeyMatrix = tuple[KeyRow, ...] | ||
NameMatrix = tuple[NameRow, ...] | ||
VMatrix = tuple[tuple[Symbol, KeyRow, NameRow], ...] |
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.
Looks good.
key_mat: KeyMatrix, | ||
name_mat: NameMatrix = None, | ||
one_indexed: bool | None = None, | ||
) -> VMatrix: |
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.
Looks good.
@@ -66,36 +76,33 @@ def from_data(symbs: tuple[str, ...], key_mat, name_mat=None, one_indexed=None): | |||
|
|||
# # V-Matrix/V-Matrix common functions (document these as z-matrix functions) | |||
# # # getters | |||
def symbols(vma, idxs: list[int] | None = None) -> list[str]: | |||
def symbols(vma: VMatrix, idxs: list[int] | None = None) -> list[str]: |
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.
The return type should be -> tuple[Symbol, ...]
(equivalent to tuple[str, ...]
).
else: | ||
symbs = () | ||
|
||
return symbs if idxs is None else tuple(map(symbs.__getitem__, idxs)) | ||
|
||
|
||
def key_matrix(vma, shift=0): | ||
def key_matrix(vma: VMatrix, shift: int = 0) -> KeyMatrix: |
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.
Looks good.
""" | ||
return len(symbols(vma)) | ||
|
||
|
||
def atom_indices(vma, symb, match=True): | ||
def atom_indices(vma: VMatrix, symb: str, match: bool = True) -> tuple[int]: |
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.
Looks good.
@@ -163,15 +167,13 @@ def atom_indices(vma, symb, match=True): | |||
return idxs | |||
|
|||
|
|||
def coordinate_key_matrix(vma, shift=0): | |||
def coordinate_key_matrix(vma: VMatrix, shift: int = 0) -> key_matrix: |
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.
The return type here is a little weird. Each value in the matrix is a tuple describing the coordinate (e.g. (2, 1) for a bond distance coordinate, (3, 2, 1) for an angle coordinate, etc.). Let's define the following above with the other types:
CoordinateKey = tuple[int, ...] | None
CoordinateKeyRow = tuple[CoordinateKey, CoordinateKey, CoordinateKey]
CoordinateKeyMatrix = tuple[CoordinateKeyRow, ...]
and then put the return type as -> CoordinateKeyMatrix:
] | ||
|
||
return tuple(map(tuple, coo_key_mat)) | ||
|
||
|
||
def coordinates(vma, shift=0, multi=True): | ||
def coordinates(vma: VMatrix, shift=0, multi=True): |
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.
With the above CoordinateKey
type defined, the return value here can be given as -> dict[Name, CoordinateKey]
.
@@ -372,7 +374,7 @@ def standard_names(vma, shift=0): | |||
|
|||
|
|||
def standard_name_matrix(vma, shift=0): |
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.
Types should be (vma: VMatrix, shift: int=0) -> VMatrix:
else: | ||
coo_dct = {name: () for name in _names} | ||
for name, coo_key in zip(_names, coo_keys): | ||
for name, coo_key in zip(_names, coo_keys, strict=False): | ||
coo_dct[name] += (coo_key,) | ||
|
||
coo_dct.pop(None) |
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.
For the functions below (using *x
to indicate function names ending in x
):
def *coordinates(vma: VMatrix) -> dict[Name, CoordinateKey]:
...
def coordinate(vma: VMatrix, name: Name) -> CoordinateKey:
...
def torsion_axis(vma: VMatrix, dih_name: Name) -> tuple[int, int]:
...
def *names(vma: VMatrix) -> tuple[Name, ...]:
...
def standard_names(vma: VMatrix, shift: int=0) -> dict[Name, Name]:
No description provided.