-
Notifications
You must be signed in to change notification settings - Fork 16
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
New geometry class and some minor fixes #78
Conversation
Fix MS-SSIM implementation and add phase-plots
Hello @bwohlberg! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-01-11 23:44:36 UTC |
Thanks for opening a PR, I have seen it, and I will take a look when I have time. |
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.
Please add some text to the docs for RegularPolygon about the default orientation. i.e. Something about how one vertex is always located at (center + radius, center)
unless that angle parameter is used.
Otherwise, LGTM! Thanks for cleaning up the docs!
Added. I was unable to check whether the text is formatted properly because the sphinx configuration doesn't actually generate the docs for I also added a setting for |
Yes, I'm trying to fix this problem in #81. I tried to setup readthedocs to validate pull requests, but the docs it generates fail with some SSL certificate error for me. |
I can access that URL without any issues. (Assuming I'm understanding the problem correctly.) |
OK, then it's being blocked by Argonne's cybersecurity policy, so I can't see it from my work station. Please look to see if the docstring is formatted how you like it. |
It looks good to me. |
Thanks for your contribution! 🔥 |
RegularPolygon
geometric entity.setup
insetup.py
.Note: the API docs for
RegularPolygon.__init__
are specified in a way that is not consistent with the rest of the package because I could not see a consistent way to include them.