Skip to content

Conversation

@mberz
Copy link
Member

@mberz mberz commented Oct 24, 2025

Changes proposed in this pull request:

  • Create an abstract base class for SphericalHarmonicDefinition for more flexible re-use in case n_max is not required or a getter only property
  • New ABC can be used as parent in Add SphericalHarmonicsAudio classes #166
  • Child classes which should not allow setting properties will need to overload the respective setter property.

@mberz mberz added enhancement New feature or request refactoring labels Oct 24, 2025
@mberz mberz self-assigned this Oct 24, 2025
@mberz mberz force-pushed the refactor/split_sh_def_base_class branch from a33dfa0 to c927ce2 Compare October 24, 2025 13:40
@mberz mberz added this to the v1.0.0 milestone Nov 14, 2025
@mberz mberz marked this pull request as ready for review November 14, 2025 17:11
@mberz mberz moved this from Backlog to Require review in Weekly Planning Nov 14, 2025
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, I found one redundancy that can maybe be removed (see comment) and have two questions:

Is testing not implemented because that done as part of the SH class?

Child classes which should not allow setting properties will need to overload the respective setter property.

Would it make sense to have two abstact base clases? One containing the properties and on the getter. This might reduce redundancy in case multiple derived classes only require getters.

@mberz
Copy link
Member Author

mberz commented Nov 17, 2025

Would it make sense to have two abstact base clases? One containing the properties and on the getter. This might reduce redundancy in case multiple derived classes only require getters.

I've thought about it, but then it would require re-writing the entire setter and checking logic to the state we had before including n_max in the base class if I remember correctly. At this point I don't think it makes sense to go back on that again.

@mberz mberz requested a review from f-brinkmann November 17, 2025 18:58
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 checking!

@github-project-automation github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Nov 18, 2025
Copy link
Member

@sikersten sikersten left a comment

Choose a reason for hiding this comment

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

Thank you! I only have two minor comments, otherwise approved.

@mberz mberz requested a review from sikersten November 19, 2025 08:23
@mberz mberz merged commit 42e30f8 into develop Nov 19, 2025
10 of 11 checks passed
@mberz mberz deleted the refactor/split_sh_def_base_class branch November 19, 2025 10:44
@github-project-automation github-project-automation bot moved this from Reviewer Approved to Done in Weekly Planning Nov 19, 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 refactoring

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants