Fixes #163; Remove Helix.pitch and Helix.yaw#166
Conversation
Completed: * Remove Helix.pitch and Helix.yaw * Update Design.from_json to be compatible with designs that specified pitch and yaw in individual Helix (except when multiple helices in the same group have different pitch/yaw values, in which case, IllegalDesignError is raised) * Updated examples that used pitch and roll * Updated unit tests that used pitch/roll * Added new unit test to test new Design.from_json Not Completed: * Add Design method for looking up pitch/yaw/roll for helices
Other changes: * Add UT to cover remaining cases * Rewrite logic for pitch yaw lookup * Refactor Design's from_json into smaller functions * Change floating_point_tolerance into private variable
|
I made the final changes and ready to merge as soon as you approve. Let me know if you see anymore issues. One thing to note is that dev is technically in process of v0.15.2. This PR would change dev version to v0.16.0. The only commit right now in dev is the one for removing the print statement. Should we just have that commit go under v0.16.0? |
Yes, just have that commit under 0.16. There's no need for a version 0.15.2. |
| for ((integerized_pitch, integerized_yaw), helix_list) in pitch_yaw_to_helices.items(): | ||
| new_pitch = group.pitch + integerized_pitch * floating_point_tolerance | ||
| new_yaw = group.yaw + integerized_yaw * floating_point_tolerance | ||
| for ((helix_pitch, helix_yaw), helix_list) in pitch_yaw_to_helices.items(): |
There was a problem hiding this comment.
Put in some comments describing which portions of this code we expect to be only needed for backwards compatibility. It's a mix between code that will always be needed for import in v0.16.0 JSON, and code that will only be executed when encountering previous versions (e.g., v0.15.0) that stored pitch and yaw in Helix. Make sure it's easy to tell the difference between those two kinds of code by adding some comments saying when something is there for backward compatibility.
dave-doty
left a comment
There was a problem hiding this comment.
Sorry, I accidentally pressed Approve instead of Request Changes last time. There's a couple comments I made requesting changes.
- Add comments to separate backward compatibility code - Fix typo in documentation string for Design.pitch_of_helix
DO NOT MERGE YET
I still have not implemented a Design method for looking up pitch/yaw/roll values for helices. Should be straightforward, but any name suggestions for the function names? Maybe
Design.{pitch/yaw/roll}_of_helix?The second reason for not merging yet is because I added code in
Design.jsonto allow for compatibility with designs that specified pitch and yaw in individual Helix. It passes all of the old test cases as well as news one I made, but I am concerned about the quality of the implementation so I would like to give @dave-doty an opportunity to review the code.Completed:
pitch and yaw in individual Helix (except when multiple helices in the
same group have different pitch/yaw values, in which case,
IllegalDesignError is raised)
Not Completed: