Skip to content
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.

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

Updated interface of TransferFunction #54

Merged
merged 11 commits into from
Jan 10, 2024
Merged

Updated interface of TransferFunction #54

merged 11 commits into from
Jan 10, 2024

Conversation

jusack
Copy link
Contributor

@jusack jusack commented May 12, 2023

The old interface of overloading getindex to use for both index access to the underlying data and frequency interpolation could lead to unwanted results, since the datatype of the passed index determined the mode.

I changed to interface to ensure that getindex will only access the underlying data and will not perform interpolation. For the frequency interpolation I made each TransferFunction object callable, similar to the access of the AbstractInterpolation object.

The new interface would look like this:

tf = TransferFunction(...)
tf[1,1] # access the tf at the first frequency component of the first channel
tf(1,1) # access the interpolated tf at a frequency of 1Hz of the first channel

This change will break some code that uses the current implementation, but I added a descriptive warning, when the getindex method fails.

@tknopp
Copy link
Member

tknopp commented May 12, 2023

This is in agreement with the convention used by https://github.com/JuliaMath/Interpolations.jl/ right?

Then I am fine with it. But we indeed would need to port the other packages then (MPIReco and MPIMeasurements). And make proper releases so that nothing breaks.

@jusack
Copy link
Contributor Author

jusack commented May 12, 2023

Yes, I am just forwarding the calls to the Interpolations.jl object that is saved in the TransferFunction, with the exception, that we are adding the channels dimensions.

I will have a look in MPIMeasurements for any places the old interface is used, since I will do some changes for the AW excitation there anyways. If someone else could check MPIReco that would be awesome.

From my side, we can wait with merging/releasing until I have a plan for my changes in MPIMeasurements

@jusack
Copy link
Contributor Author

jusack commented May 16, 2023

As another proposed change I added support for Unitful units to the TransferFunction object. The units are saved per channel in a vector in the struct and are applied when using frequency interpolation. When saving the units there are saved as a parsable string in the h5 file.

The change should be fully backwards compatible, assuming no units for every TransferFunction read from a h5 file, where no unit key is found. If no units are defined, nothing should change to the current behaviour.

To simplify the construction and to avoid confusion between the amp/phase and the complex valued constructor, I changed some of the function signatures to use keyword arguments. That could potentially break external code.

Should we add a keyword option or trait to the tf() call to enable the removal of units or should the user just use ustrip when handling a TransferFunction with units?

@jusack
Copy link
Contributor Author

jusack commented Jan 8, 2024

From my side this PR would be ready for release. I added additional tests and wrote a documentation chapter on the TransferFunction type. Maybe we can have a quick discussion/review if we still need to fix something in this PR or elsewhere..

@jusack jusack requested a review from nHackel January 8, 2024 15:34
@nHackel nHackel merged commit 8a40dfc into master Jan 10, 2024
6 checks passed
@nHackel nHackel deleted the JA/transferfunction branch January 10, 2024 10:14
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