Skip to content

Conversation

@mberz
Copy link
Member

@mberz mberz commented Aug 8, 2025

This additional class could be used to provide the definition of the sperical harmonics to other classes.
The new class already serves as base class for the SphericalHarmonics class.

Requires #172

Todo:

  • Update to new monopole normalizations
  • Add order (optional)

@mberz mberz marked this pull request as draft August 8, 2025 16:08
@mberz mberz added the enhancement New feature or request label Aug 22, 2025
@mberz mberz moved this from Backlog to Drafting Phase in Weekly Planning Aug 22, 2025
@mberz mberz added this to the v1.0.0 milestone Aug 22, 2025
@mberz mberz marked this pull request as ready for review September 17, 2025 09:32
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 the effort. This will be very nice to have. Looks like many comments, but they are all rather small and and in some cases repeating.



class SphericalHarmonics:
class SphericalHarmonicDefinition:
Copy link
Member

Choose a reason for hiding this comment

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

For discussion: Should this be public or private? From the top of my head, I did not see any use for it if its not connected to any data (SH basis or Ambisonics signal).

Copy link
Member Author

Choose a reason for hiding this comment

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

Public, since it can for example be used to create transforms, such as rotations/translations

Copy link
Member

Choose a reason for hiding this comment

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

disscussion over 😂

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, would depend on how these will be implemented. The current draft of the SHT would not need it, I think. We could also keep it private until we need it to be public.

Copy link
Member

Choose a reason for hiding this comment

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

SHT woulnt need it, but SphericalHarmonics and SpericalHarmonicsSignal, SpericalHarmonicsTimeData and SpericalHarmonicsFrequencyData would need it. in this way they save a lot of duplicated code.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, of course - I was only wondering about public vs. private. Public is probably not required for those examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote translations/rotations, not SHT ;)

Attributes
----------
condon_shortley : bool
Copy link
Member

Choose a reason for hiding this comment

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

Does not document condon_shortley = 'auto'. Maybe not needed it class would be private, but nice to have in any case.

@sikersten
Copy link
Member

For completeness, this somehow PR relies on #151. Or should we add the classes to the docs here?

@f-brinkmann
Copy link
Member

We discussed that it would be good to add spharpy/classes/sh.py to the docs for ease of review. The final docs should still be done in #151.

@mberz
Copy link
Member Author

mberz commented Sep 26, 2025

I'm currently waiting for #172 to be ready and merged to continue working on this PR.

@ahms5 ahms5 removed request for ahms5 and tluebeck October 8, 2025 10:21
@ahms5 ahms5 removed the request for review from sikersten October 8, 2025 10:21
@ahms5
Copy link
Member

ahms5 commented Oct 8, 2025

it is merged now, please rerequest review if its ready

@mberz
Copy link
Member Author

mberz commented Oct 9, 2025

superseded by #205

@mberz mberz closed this Oct 9, 2025
@github-project-automation github-project-automation bot moved this from Drafting Phase to Done in Weekly Planning Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants