search

MagneticParticleImaging / MDF

Magnetic Particle Imaging Data Format
Other
18 stars 10 forks source link

Change flag isFourierTransformed to isFrequencyDomain #90

Closed Neumann-A closed 7 years ago

Neumann-A commented 7 years ago

Since isFourierTransformed does not convey the meaning that the FT has been done in frequency domain (it could be spatial domain see #89) I propose to change the flag to isFrequencyDomain and may add another flag which is isSpatialFrequencyDomain depending on the outcome of #89

tknopp commented 7 years ago

This is just a documentation issue (is it?) isFourierTransformed means that a Fourier transformation along the time axis is done. If this is not clear, please improve documentation. isFrequencyDomain is even less specific. One could think about isTemporalFourierTransform but I don't see the need for that.

Neumann-A commented 7 years ago

This is just a documentation issue (is it?)

No renaming flags will become a braking change so you have only one chance to get the naming right. If MDF should in the future support #89 isFourierTransformed becomes ambiguous and so a new name must be found beforehand. Sry has nothing to do with the quality of the documentation. Although isFrequencyDomain is not that unambiguous in combination with isSpatialFrequencyDomain it can be at least documented without ambiguity. We could fall back to string field describing it but flags are easier to check programmatically. If you dont like the propsed flags maybe those: isFourierTransformedTime and isFourierTransformedSpace

I chose isFrequencyDomain because v1 had the data datasets as data and dataFD

tknopp commented 7 years ago

why is isFourierTranformed ambiguous while isFrequencyDomain is not? I don't get you.

tknopp commented 7 years ago

The one describes a space, the other the transformation how to get there.

tknopp commented 7 years ago

No renaming flags will become a braking change so you have only one chance to get the naming right

We would not rename anything. We define it now once for all ("isFourierTransformed" implies that a real to complex FFT has been applied) and then its neither ambiguous nor will we change it.

MandyA commented 7 years ago

Is this issue solved?

Neumann-A commented 7 years ago

why is isFourierTranformed ambiguous while isFrequencyDomain is not? I don't get you.

I wrote:

Although isFrequencyDomain is not that unambiguous in combination with isSpatialFrequencyDomain it can be at least documented without ambiguity.

There is another point here: Frequency (Hz) is from a physics perspective automatically bound to the inverse of time. If you mean some other freqeuncy you need to specify it in the naming e.g. spatial frequency. isFourierTransformed is too general in the MPI and Imaging context since both fourier transformation are used. In MPI you could argue ok we usally mean the Fourier transform in time but in the Imaging domain the spatial fourier transformation is also a thing. So the user never knows whether isFourierTransformed is meant to be interpreted in the time or the spatial domain without reading the spec in detail. I would rather have more verbose flags then needing the user to get every detail of the spec. Another reason why isFourierTransformed might not be a good name is that the user might assume that he can simply back transform it to get the time data back which is not true if a frequency selection has already happened.

We would not rename anything. We define it now once for all ("isFourierTransformed" implies that a real to complex FFT has been applied) and then its neither ambiguous nor will we change it.

Of course the spec can always smash down the hammer to define things.

i dont mind naming of the flag too much i could also live with isSpatialFourierTransform if #89 makes it into the spec. I just thought that isFourierTransformed is too general in the context we are working and describing the data. (Reasons see above)

tknopp commented 7 years ago

frequency is exactly as ambiguous as Fourier. For some reason you seem to believe that the default for frequency is Hz but for Fourier it is not.

I personally don't have enough time to bikeshed each variable name like this (where it is only a very subjective perspective). We favored to describe the transformation here and not the domain. Therefore, our decision.