Skip to content

Conversation

@tluebeck
Copy link

@tluebeck tluebeck commented Jun 3, 2025

Which issue(s) are closed by this pull request?

Closes #133

Changes proposed in this pull request:

@tluebeck tluebeck self-assigned this Jun 3, 2025
@tluebeck tluebeck added the enhancement New feature or request label Jun 3, 2025
@xefonon
Copy link
Member

xefonon commented Jun 14, 2025

@tluebeck #101 has been merged! 👍

@tluebeck tluebeck marked this pull request as ready for review July 1, 2025 07:16
@f-brinkmann f-brinkmann added this to the v1.0.0 milestone Jul 2, 2025
@f-brinkmann f-brinkmann moved this from Backlog to Require review in Weekly Planning Jul 2, 2025
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 think we should review the concept again. Specifying all properties again does not use the power of the SH class at all from a users perspective.
Passing the SH class instead of creating it within the function would make things much easier in my opinion.

@mberz mberz moved this from Require review to Open Discussion in Weekly Planning Jul 2, 2025
@f-brinkmann
Copy link
Member

  • We decided that the functions should get an SH class object.
  • @tluebeck will check if it is easy to have SHTimeData and SHFrequencyData by changing inheritance.

spharpy/sht.py Outdated
Comment on lines 58 to 65
if not signal.time.shape[axis] == coordinates.csize:
if coordinates.csize not in signal.time.shape:
raise ValueError("Signal shape does not match "
"number of coordinates.")
else:
axis = signal.time.shape.index(coordinates.csize)
warnings.warn("Compute spherical harmonics transform along "
f"axis = {axis}.", UserWarning)
Copy link
Member

@f-brinkmann f-brinkmann Jul 7, 2025

Choose a reason for hiding this comment

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

I think we should only raise an error here and not change the user input. But I like the idea of detecting the axis. How about having axis='auto' as a default and use

axis = np.where(np.array(signal.cshape) == Y_inv.shape[1])[0]

to detect the axis. Compared to signal.time.shape.index(coordinates.csize) it has the advantage of detecting the case of multiple matching axes. If len(axis) == 1 we proceed. If len(axis) == 0 we raise an error and if len(axis) > 1 we raise a different errror.

@f-brinkmann f-brinkmann moved this from Open Discussion to Implementation in progress in Weekly Planning Jul 25, 2025
@f-brinkmann f-brinkmann modified the milestones: v1.0.0, v1.1.0 Sep 12, 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 feature

Projects

Status: Implementation in progress

Development

Successfully merging this pull request may close these issues.

5 participants