Closed alexjaffray closed 1 year ago
Base: 66.68% // Head: 65.79% // Decreases project coverage by -0.89%
:warning:
Coverage data is based on head (
b1334e9
) compared to base (4e2095a
). Patch coverage: 79.16% of modified lines in pull request are covered.:exclamation: Current head b1334e9 differs from pull request most recent head db68537. Consider uploading reports for the commit db68537 to get more accurate results
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I slightly changed the interface since the function can determine itself, if the applied imsize matches the encoding size.
Then I also changed the error calculation. Our nrmsd actually is a little bit too clever since it determines an optimal scaling before error calculation. If I remove this (last commit) one can see that one of your test (testESPIRiT_newSize(128)
) is failing. It turns out that the sensitivity maps are simply negative: Do you have an idea where this comes from?
Thanks for changing the interface, I think your solution is quite elegant. I'm not sure why the maps are simply negative, this is strange to me, I am looking into this further.
@tknopp it seems that there is simply a constant phase offset between the two sensitivity maps of approximately pi. As we already do in the existing ESPIRiT test, we should correct for this in the new test for consistency. Currently this is done with the code:
phs = mean(angle.(smaps))
phs2 = mean(angle.(smaps2))
smaps2 = exp(1im*(phs-phs2)) .* smaps2
However, it seems to me that this correction is not wrap-safe... and our current test merely avoids the issue by coincidence, which is not good. Using N = 32, or most odd numbers N as the argument to testESPIRiT()
demonstrates this. For example, if the phase of smaps
is roughly zero, and the phase of smaps2
is centered roughly around pi, then both phs
and phs2
will be close to zero, causing the constant offset correction to fail.
I am wondering if there is a better way of accounting for constant phase offsets in the test, and also wondering why they happen for otherwise supposedly identical data.
Thanks for investigating. I had actually not seen that the original espirit test ignored constant phase offsets. There is indeed a better method for that, which is wrap free. You will find it in my last commit. It's just linear regression on complex numbers.
I will merge this now. However, I actually can't judge right now whether we still have an underlying problem in espirit itself since it is not nice to get these phase changes just when changing the input dimensionality. If this is a bug, it would great if you or @JakobAsslaender could open an issue describing the problem so that we are at least aware of the issue.
Not sure that I am the right person to open an issue since I don't fully understand the problem. Once an issue with a MWE is filed, I might be the right person to fix it though ;). At some point we are lazy and write the calibration k-space in the corner of a larger zero-array. This is not really a problem, but it would not surprise me if it leads to global phase shifts. If anyone writes a MWE, it would be of tremendous help if you could do that directly with the function espirit(x::Array)
instead of the wrapper function spirit(x::AcqData)
.
Added functionality to
espirit
enabling sensitivity maps estimated at arbitrary sizes by zero padding low resolution k-space data, as mentioned in the meeting. There is possibly a more elegant way of doing this, but this seems to work. Tests pass both in the original ESPIRiT test (which I've made into a function) and the new tests I've added which test that the same maps are produced from nominal resolution acqData and lower resolution acqData.