Skip to content
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

Bug fixes [Issue-136], [Issue-137] #138

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jcao-bdai
Copy link
Collaborator

Bug-fixes:

  • Issue-136: missing args (thanks to report by @ManuelZ);
  • Issue-137: incorrect type usage (thanks to report by @alsaibie);
  • minor: python version bump in sphinx worker.

Copy link
Collaborator

@myeatman-bdai myeatman-bdai left a 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.

Copy link
Collaborator

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.

@petercorke
Copy link
Collaborator

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants