Skip to content

Real Spherical Harmonics Directivity#422

Merged
fakufaku merged 8 commits into
LCAV:masterfrom
je-fleischhauer:erik/shdirectivity
Feb 3, 2026
Merged

Real Spherical Harmonics Directivity#422
fakufaku merged 8 commits into
LCAV:masterfrom
je-fleischhauer:erik/shdirectivity

Conversation

@je-fleischhauer

Copy link
Copy Markdown
Contributor

Thanks for sending a pull request (PR), we really appreciate that! Before hitting the submit button, we'd be really glad if you could make sure all the following points have been cleared.

Please also refer to the doc on contributing for more details. Even if you can't check all the boxes below, do not hesitate to go ahead with the PR and ask for help there.

  • Are there docstrings ? Do they follow the numpydoc style ?
  • Have you run the tests by doing nosetests or py.test at the root of the repo ?
  • Have you checked that the doc builds properly and that any new file has been added to the repo ? How to do that is covered in the documentation.
  • Is there a unit test for the proposed code modification ? If the PR addresses an issue, the test should make sure the issue is fixed.
  • Last but not least, did you document the proposed change in the CHANGELOG file ? It should go under "Unreleased".

Happy PR 😃

I added this functionality to pyroomacoustics and thought it would be worthwhile to share it with others.
Since there is a copyright notice in the init file of the directivity folder, I added the function to the experimental folder.

@fakufaku

Copy link
Copy Markdown
Collaborator

Hi @je-fleischhauer , this is really cool, thank you so much!

I think it would be great to have this in the directivity package.
If you are ok with it, please add the same copyright as in the init to your file, but replace my name by yours (or github username if you prefer).

@fakufaku fakufaku left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey, thanks for sharing this. This is very cool.

Would you mind running the linters so that the CI pass?

pip install black isort
black .
isort --profile black .

I think you also need to merge with master to fix the tests failing due to a deprecation in numpy that was just fixed.

Comment thread pyroomacoustics/directivities/harmonics.py
def get_mn_in_acn_order(order):
"""Calculates the (m,n) pairs in ACN order up to a given order.

Parameters:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use the numpydoc docstring style for consistency in the package.
https://numpydoc.readthedocs.io/en/latest/format.html

Please fix all the docstrings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hope, I corrected this so it is compatible with the numpydoc docsting style.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think "Parameters" and "Returns" should not have a colon after and should be underlined with hyphens.
See https://numpydoc.readthedocs.io/en/latest/format.html#parameters

This is important for the auto-generated documentation to be formatted correctly.

@fakufaku fakufaku Jan 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@je-fleischhauer , I deeply apologize for the nit-picking, but there mustn't be a colon at the end of the sub-titles.
E.g., the following is correct,

Parameters
----------

while this is incorrect (note the the colon at the end)

Parameters:
-----------

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Everything's fine, I'm learning something here 😄

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you!

Comment thread pyroomacoustics/experimental/harmonics.py Outdated
@mattpitkin

Copy link
Copy Markdown
Contributor

@je-fleischhauer You'll probably want to rebase/merge this branch with master. I made a fix in #423 that fixes a numpy issue for Python >3.10 and should fix the current CI test failures that are showing up.

@je-fleischhauer

Copy link
Copy Markdown
Contributor Author

@je-fleischhauer You'll probably want to rebase/merge this branch with master. I made a fix in #423 that fixes a numpy issue for Python >3.10 and should fix the current CI test failures that are showing up.

I rebased and merged my branch with the master branch. My system now passes the CI test.

"""
Calculates the real spherical harmonics.

Parameters:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto

condon_shortley_phase : bool, optional
If True, includes the Condon-Shortley phase factor (-1)^m. Default is False.

Returns:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto

order : int
Maximum degree of the spherical harmonics.

Returns:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto

@fakufaku fakufaku left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you. I think we are almost there. You just need to fix the docstrings to follow numpy style, which is important for auto-generating the doc.

@fakufaku fakufaku merged commit 61731e2 into LCAV:master Feb 3, 2026
24 checks passed
@fakufaku

fakufaku commented Feb 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks you @je-fleischhauer ! 🤗

@fakufaku

fakufaku commented Feb 4, 2026

Copy link
Copy Markdown
Collaborator

@je-fleischhauer I have merged everything now into master branch. I was wondering if you would be willing to do a new PR to add docstrings at the top of pyroomacoustics/directivities/harmonics.py to explain what real spherical harmonics are and how to use them. I've added some placeholder for now, but I think this would be very useful to have documentation for that part as I don't think this is very well known.

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