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

Remove special treatment of DC in tfCorrection + fix load_TF_fromVNA #100

Merged
merged 12 commits into from
Dec 6, 2024

Conversation

jusack
Copy link
Contributor

@jusack jusack commented Sep 9, 2024

For inductive measurements the DC component gets lost, this would mean a real zero in the TF which can not be recovered.

At some point this has been handled by a keyword parameter tfDCCorrection, deciding if the TF should be applied to the DC components. This was changed to automatically detecting if all channels of the TF contain no zeros (or NaNs) and only then dividing by the TF-DC-value.

This leads to the following issues:

  • If one TF channel has a zero in it (which would be the correct definition of the TF) the DC component does not get touched at all, meaning a DC offset that was numerically small in the V scale will be much bigger in the Am² scale the data is in after the correction resulting in a undefined behaviour
  • One channel can deactivate the DC correction for other channels (e.g. channels that can actually detect DC and would need this correction)
  • The workaround of just setting the DC component of the TF to a very high value to suppress the DC value leads to issues with the interpolation of the TF and, depending on the arbitrarily chosen value, to not so nice plots on a logarithmic scale if the DC component is orders of magnitude smaller than the noise floor of the measurement

Due to these reasons I want to propose just dropping the check for zeros in the TF all together as implemented in this PR. This results in the following:

  • If the TF contains a zero (or NaN) the resulting value in frequency domain will be NaN, I tested plotting with CairoMakie (as used in MPIUI) and this value is just ignored in plotting. This is probably preferable to setting the value to zero, because the logarithmic scale breaks with a true zero
  • For a time domain signal the DC component is set to from NaN to zero before inverse Fourier transform to have a time domain representation without any DC component

The only issue I could see with this is that a NaN value in the frequency domain data has to be set to zero manually if the user wants to inverse fourier transform the data themselves, potentially breaking currently working scripts

What are your thoughts on this change? Does this break something else?

Copy link
Member

@jonschumacher jonschumacher left a comment

Choose a reason for hiding this comment

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

Looks good to me.

src/TransferFunction.jl Show resolved Hide resolved
src/Measurements.jl Outdated Show resolved Hide resolved
@tknopp
Copy link
Member

tknopp commented Sep 10, 2024

I think I would give this a thumbs up as well. My only concern would have been a breakage at later stages (MPIUI and/or reconstruction) but if issues arise there, we can fix them there.

@nHackel: any objections?

@nHackel
Copy link
Member

nHackel commented Sep 10, 2024

I think this change sounds good! It might be a breaking change, but most recos will probably be fine, since we would have a minFreq anyway/no good SNR for the offset freq (I think).

This, together with #96 and the potential TF change in MPIMeasurements.jl could be packaged as a new treatment of frequencies with no backward compatibility. If we go that route, we should have migration guides in the releases and also check if there are any other breaking changes we want to do

@jusack jusack changed the title Remove special treatment of DC in tfCorrection Remove special treatment of DC in tfCorrection + fix load_TF_fromVNA Sep 11, 2024
@jusack
Copy link
Contributor Author

jusack commented Sep 11, 2024

This is not directly related to the original scope of the PR, but I also added new methods for combining TFs (which I need in MPIMeasurements when combining the channel TFs to form the TF to be written into the MDF) and split up the load_tf_fromVNA function into multiple functions.
The reading of the datafile is now done in readVNAdata while the processing of the TF using the calibration coil parameters is done in processRxTransferFunction both of which are called by load_tf_fromVNA. Right now the keyword arguments R,N and A are functional again in load_tf_fromVNA, however, it would be easy to deactivate them for load_tf_fromVNA and just not pass them to processRxTransferFunction. What are your thoughts? Should we "fix" load_TF_fromVNA or keep the behaviour stable?

@jusack jusack linked an issue Sep 11, 2024 that may be closed by this pull request
@nHackel nHackel merged commit f0274d9 into master Dec 6, 2024
7 checks passed
@nHackel nHackel deleted the JA/zero-in-tf branch December 6, 2024 11:23
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.

Keyword parameters ineffective
4 participants