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

New geometry class and some minor fixes #78

Merged
merged 15 commits into from
Jan 12, 2022

Conversation

bwohlberg
Copy link
Contributor

@bwohlberg bwohlberg commented Jan 6, 2022

  • Add a RegularPolygon geometric entity.
  • Fix some typos and formatting issues in the API docs.
  • Add some missing attributes in setup in setup.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.

@pep8speaks
Copy link

pep8speaks commented Jan 6, 2022

Hello @bwohlberg! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 552:36: W605 invalid escape sequence '\p'
Line 553:13: W605 invalid escape sequence '\p'

Comment last updated at 2022-01-11 23:44:36 UTC

@carterbox carterbox self-assigned this Jan 6, 2022
@carterbox
Copy link
Contributor

Thanks for opening a PR, I have seen it, and I will take a look when I have time.

@carterbox carterbox self-requested a review January 6, 2022 20:28
@bwohlberg
Copy link
Contributor Author

I have not done any testing to confirm, but I expect that 31eb3cf will resolve #77.

Copy link
Contributor

@carterbox carterbox left a 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!

@carterbox carterbox changed the base branch from master to 0.5.x January 11, 2022 18:15
@bwohlberg bwohlberg requested a review from carterbox January 11, 2022 20:41
@bwohlberg
Copy link
Contributor Author

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.

Added. I was unable to check whether the text is formatted properly because the sphinx configuration doesn't actually generate the docs for __init__.

I also added a setting for bibtex_bibfiles sphinx conf.py in an attempt to address the failure of the readthedocs build workflow.

src/xdesign/geometry/area.py Outdated Show resolved Hide resolved
@carterbox
Copy link
Contributor

I also added a setting for bibtex_bibfiles sphinx conf.py in an attempt to address the failure of the readthedocs build workflow.

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.
https://xdesign--78.org.readthedocs.build/en/78/

@bwohlberg
Copy link
Contributor Author

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. https://xdesign--78.org.readthedocs.build/en/78/

I can access that URL without any issues. (Assuming I'm understanding the problem correctly.)

@carterbox
Copy link
Contributor

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.

@bwohlberg
Copy link
Contributor Author

It looks good to me.

@carterbox carterbox merged commit bf130c7 into AdvancedPhotonSource:0.5.x Jan 12, 2022
@carterbox
Copy link
Contributor

Thanks for your contribution! 🔥

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