-
Notifications
You must be signed in to change notification settings - Fork 96
Bug fixes [Issue-136], [Issue-137] #138
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
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.
We should change the plot_ellipsoid and all the other plotting functions of this basic shapes to accept pose and pass them into_render3d in the same way you have for `plot_sphere.
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.
Python 3.8 is already at EOL and may disappear from the GH environment at any time. Suggest we go for something like 3.12 so we don't have to touch this again for a few years.
Also note that checkout action is currently at v4, I don't know what the difference is, or whether the old ones eventually become deprecated.
We should check the python and action versions across all the workflow files, for sphinx building and code coverage.
We should change the
plot_ellipsoidand all the other plotting functions of this basic shapes to accept pose and pass them into_render3din the same way you have for `plot_sphere.
As is, plot_ellipsoid is completely general, centroid given by centre and shape and orientation by E. pose is not needed and I don't think @jcao-bdai has changed it. We can discuss this more.
In general, adding a pose argument and passing it through is a good thing, because it documents that capability for each plot_xxx function. That capability did exist because a pose argument was picked up by kwargs and passed through to _render, but that's pretty opaque. Bottom line, this is a good addition, but maybe not for plot_ellipse.
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.
Approving to close out these changes.
Bug-fixes: