Skip to content

Bug: Doc: clean up imports in doc #136

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.

Sign up for GitHub

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

Jump to bottom
Merged
mberz merged 15 commits into from
Sep 26, 2025
Merged

Bug: Doc: clean up imports in doc #136

mberz merged 15 commits into from
Sep 26, 2025

Conversation

@ahms5
Copy link
Member

@ahms5 ahms5 commented Jun 1, 2025 •
edited
Loading

Changes proposed in this pull request:

edit 1.6.25:

  • added indexing module to docs accroing to @mberz review
  • fixes footnotes warnings to prevent test to fail
    • changes were made, sothat the first sentence of the docstring does not contain a reference, this leads to problems within autofootnote.

edit 25.7.

  • What would be the best place in the documentation for indexing and spherical

edit 12.9.

  • all submodules are also in the doc

edit 19.9.:

@ahms5 ahms5 added this to the v1.0.0 milestone Jun 1, 2025
@ahms5 ahms5 added the documentation Improvements or additions to documentation label Jun 1, 2025
@ahms5 ahms5 moved this from Backlog to Require review in Weekly Planning Jun 1, 2025
@ahms5 ahms5 self-assigned this Jun 1, 2025
@ahms5 ahms5 mentioned this pull request Jun 1, 2025
Merged
f-brinkmann
f-brinkmann reviewed Jun 2, 2025
View reviewed changes
Copy link
Member

@f-brinkmann f-brinkmann 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 maybe discuss if spharpy.sphecial should be considered a private module. For example the spherical_harmonics basis methods are barebone wraps of corresponding scipy methods with less documentation than in the SH classes. As far as I understand it, they are used as helpers only and such similar functions defined in multiple places might be confusing.

@ahms5 ahms5 moved this from Require review to Open Discussion in Weekly Planning Jun 3, 2025
@ahms5
Copy link
Member Author

ahms5 commented Jun 15, 2025

I have removed it for now. maybe in #151 we can add it again, if we see that we will need it.

@ahms5 ahms5 requested a review from f-brinkmann June 15, 2025 16:15
f-brinkmann
f-brinkmann reviewed Jun 15, 2025
View reviewed changes
@ahms5 ahms5 requested a review from f-brinkmann June 15, 2025 19:23
mberz
mberz requested changes Jul 3, 2025
View reviewed changes
Copy link
Member

@mberz mberz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree here. If we remove it from the docs people typically do not know about the existence of it. So please re-add it.

@github-project-automation github-project-automation bot moved this from Open Discussion to Require review in Weekly Planning Jul 3, 2025
@ahms5
Copy link
Member Author

ahms5 commented Jul 4, 2025

I agree, we should add what we have. should we simply close it and add the ci doc tests with warning i a dedicated pr ?

@ahms5
Copy link
Member Author

ahms5 commented Sep 12, 2025

currently all spharpy modules are in the doc with this pr. ready for review

f-brinkmann
f-brinkmann approved these changes Sep 15, 2025
View reviewed changes
Copy link
Member

@f-brinkmann f-brinkmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care. I think this would give a good basis for decided on the final structure of the docs at a later point.

@ahms5 ahms5 requested review from mberz and removed request for mberz September 15, 2025 14:44
mberz
mberz requested changes Sep 16, 2025
View reviewed changes
docs/api_reference.rst Outdated
.. toctree::
:maxdepth: 1

Copy link
Member

@mberz mberz Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not add blank spaces (if you use the editorconfig plugin it will remove trailing whitespaces automatically ;))

spharpy/spherical.py Outdated
This implementation avoids singularities at the poles using identities
derived in [#]_.
derived in [#]_ and [#]_.
Copy link
Member

@mberz mberz Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect, the singularity avoiding formulation is detailed only in the second reference.

Copy link
Member Author

@ahms5 ahms5 Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mberz I tried to fix it. But I'm not into the topic, if this isnt correct, or you have an better idea, feel free to change here in the PR directly

Copy link
Member

@mberz mberz Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@github-project-automation github-project-automation bot moved this from Open Discussion to Require review in Weekly Planning Sep 16, 2025
ahms5
ahms5 commented Sep 19, 2025
View reviewed changes
ahms5
ahms5 commented Sep 19, 2025
View reviewed changes
@ahms5 ahms5 requested a review from mberz September 19, 2025 13:30
@ahms5
Copy link
Member Author

ahms5 commented Sep 19, 2025

ready for review

hoyer-a
hoyer-a approved these changes Sep 25, 2025
View reviewed changes
Copy link
Member

@hoyer-a hoyer-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question, otherwise approved.

docs/modules/spharpy.special.rst

.. automodule:: spharpy.special
:members:
:special-members: __init__
Copy link
Member

@hoyer-a hoyer-a Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why this was added, since currently there are no classes inside special

@github-project-automation github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Sep 26, 2025
@mberz mberz merged commit 502ec57 into develop Sep 26, 2025
9 of 11 checks passed
@github-project-automation github-project-automation bot moved this from Reviewer Approved to Done in Weekly Planning Sep 26, 2025
@mberz mberz deleted the bug/cleanup_doc branch September 26, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mberz mberz mberz approved these changes

@f-brinkmann f-brinkmann f-brinkmann approved these changes

@hoyer-a hoyer-a hoyer-a approved these changes

@sikersten sikersten Awaiting requested review from sikersten

Assignees

@ahms5 ahms5

Labels

documentation Improvements or additions to documentation

Projects

Weekly Planning
Status: Done

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ahms5 @mberz @f-brinkmann @hoyer-a