Integrate Segway Toolkit into Segway#181
Integrate Segway Toolkit into Segway#181isaiahah wants to merge 96 commits intohoffmangroup:developfrom
Conversation
EricR86
left a comment
EricR86
left a comment
There was a problem hiding this comment.
Let me know when you've addressed the changes.
Also can you address the posterior trackfiles in the simplesubseg touchstone folder? Why are they there?
michaelmhoffman
left a comment
michaelmhoffman
left a comment
There was a problem hiding this comment.
Great work, will re-review
| @@ -1,5 +1,2 @@ | |||
| % XXXopt: good candidate for a linear combination C deterministic mapper | |||
There was a problem hiding this comment.
Can you please restore this comment?
There was a problem hiding this comment.
Actually, it might be better to move it to whatever Python code calls this so it isn't copied into every project.
There was a problem hiding this comment.
I've moved this comment (verbatim) to the Python code which loads this file. See input_master.py line 121.
There was a problem hiding this comment.
How about just preamble.tmpl?
| """ | ||
| # Ensure all arguments belong to the correct type | ||
| if not all(isinstance(arg, NumericArrayLike) for arg in args): | ||
| if not all((isinstance(arg, float) or isinstance(arg, int) or |
There was a problem hiding this comment.
Also, please replace with at least the tuple (float, int, ndarray). That has been supported for a long time, even if union types won't be directly until Py3.10.
I think NumericArrayLike.get_args() which should get you (float, int, ndarray) should be even better, because it eliminates future potential inconsistency issues.
Please add a comment indicating the issue with NumericArrayLike and at what Python version it won't be an issue anymore
There was a problem hiding this comment.
I've added checking type with the tuple and a comment explaining why. I've used typing.get_args(NumericArrayLike) to print the types.
| raise TypeError("Argument has incompatible type. " | ||
| f"Expected {NumericArrayLike}") | ||
| raise TypeError("Argument has incompatible type." | ||
| "Expected float, int, or ndarray.") |
There was a problem hiding this comment.
Reverse if possible
There was a problem hiding this comment.
I've reversed the order of these sentences.
| # If union iterable, fix. Otherwise, hardwrite | ||
| raise TypeError("Argument has incompatible type. " | ||
| f"Expected {NumericArrayLike}") | ||
| raise TypeError("Argument has incompatible type." |
There was a problem hiding this comment.
Need a space between sentences
| # if stored items are generic strings, return them out without | ||
| # any additional formatting. Otherwise, apply formatting | ||
| if self.kind == OBJ_KIND_GENERICSTRING: | ||
| lines += self.get_unformatted_lines() |
There was a problem hiding this comment.
Don't use += with a list. Switch to lines.extend() or lines.append().
There was a problem hiding this comment.
Changed throughout.
| if not isinstance(value, OneLineKind): | ||
| # Unless a class where all data is on one line (OneLineArray and | ||
| # VirtualEvidence), write the object's remaining lines | ||
| if not (isinstance(value, OneLineArray) or |
There was a problem hiding this comment.
isinstance(value, (OneLineArray, VirtualEvidence))
| Attributes: | ||
| mean: the InlineSection object stored at InputMaster.mean | ||
| covar: the InlineSection object stored at InputMaster.covar | ||
| mean: InlineSection object which point to InputMaster.mean |
There was a problem hiding this comment.
which point to -> referred to by
There was a problem hiding this comment.
Changed throughout.
| # if line_before is set, use it to begin the section's lines | ||
| lines = [] | ||
| if self.line_before: | ||
| lines += [self.line_before] |
There was a problem hiding this comment.
same as before with .append(), .extend()
There was a problem hiding this comment.
Changed throughout.
| @@ -0,0 +1,503 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
There was a problem hiding this comment.
For the next round, can you please fix this so that Git knows what it should diff against? It looks like this is modification of old code and I don't think anyone wants us to review it all de novo
There was a problem hiding this comment.
This file was previously segway_input_master.py. The helper functions and opening section are the only things which were not changed. I haven't yet found a way to change how Git generates the diff, but will at minimum specify the relevant line sections in the next round.
3 participants
No description provided.