Fixed inverse kinematics#214
Conversation
…sociation/ARTIST into features/combined_optimization
…sociation/ARTIST into features/combined_optimization
There was a problem hiding this comment.
LGTM, I think we are now very close to release-ready. A couple of things to be addressed before we do the merge:
-
I checked the inverse kinematics and all related changes carefully. The implemented functionality matches the formulae provided by @Filos1992 and makes sense to me.
-
I made some small changes for obvious things, like minor modifications to docstrings or comments etc. The commit suggestions I was not sure about I left unchanged to be addressed once @MarleneBusch is back from vacation.
-
Some methods (I made specific comments there) would really benefit from further refactoring into helper functions to improve clarity and readability of the code.
-
I am very unsure about whether we should have plotting functionality implemented at such a deep level inside the package as it is the case for the
KinematicsReconstructorclass. Can we move these "analysis" methods somewhere else? -
I think we should really move all scripts related to the full-field optimization to another repo inside the ARTIST Association that just imports
artistas a package. We should have done the same for the PAINT plots; however, we cannot move these scripts anymore as they are explicitly linked in the Nature Energy paper.
Note that I did not check the plotting and example scripts properly.
for more information, see https://pre-commit.ci
2 participants
Description
The main feature of this PR is the fixed inverse kinematics.
Fixes #205
Fixes #213 (there is now only one .pt file storing all tensors used in the pytests in a dict, this reduces 35 files into 1 file, it is slightly less userfriendly maybe, so if this change is not wanted I can revert it.)
Type of change
Checklist: