Downsample Bug

[j-c-c]

This resolves the issues found in #1155. See issue for results of the changes in this PR.

The main changes in this PR are:

1. Use centered_fftn for Volume.downsample:
2. Option to not zero out nyquist frequency for even resolution in Image.project and Image.backproject. Set to not zero out by default.
3. Resolve some hardcoded test failures related to the above changes.
4. Improve tolerance for some tests that were using project with nyquist frequencies zero'd out.

[garrettwrong]

Thanks for tracking this down. Couple cleanup items, but overall very good thanks!

[garrettwrong]

Up to you regarding the method overload vs hiding option from user. Either should work well.

I think this can move forward as it is obviously an improvement that we want.

However, I have a nagging concern about both our 2d and 3d downsamples. They don't seem to agree very well (IMO) with an idealized real space downsample. Consider this function. (It's easily extended to more dimensions).

def ds2(x, bs=2):
    x = x.reshape(L//bs, bs, L//bs, bs)
    x = np.mean(x, (1,3))  # traditionally I think would be a sum here, but... regardless...
    return x

I looked at it as a sanity reference when we were chatting about the shifting/centering yesterday, and then decided not to look anymore 😇 🙈 .

[garrettwrong]

Don't think you need to specify axes here.

[garrettwrong]

Did you need the axes after all?

[j-c-c]

Oops. Thought I removed those. They are now removed :)

[garrettwrong]

There are ways to avoid this extra argument; mentioned one in other remark.

If you keep the flag, Joakim can let you know if he prefers to continue using the other var name (zero_nyquist).

[j-c-c]

Thought about using zero_nyquist too, but went with legacy so it would be more apparent that it would need to be removed if we eventually do away with _LegacySimulation.

[garrettwrong]

I figured, might not need. No worries.

[j-c-c]

@janden, I made the changes that we discussed during our last dev meeting. Namely, that we default to zeroing out the Nyquist frequencies in Volume.project and Image.backproject for even resolutions. And also, adding a zero_nyquist flag to Image.downsample and Volume.downsample, which by default zeroes out the Nyquist frequency when downsampling into even resolution.

These changes maintain our centering convention and also have the property that vol.project().downsample() == vol.downsample().project().

However, there are definitely artifacts introduced by removing the Nyquist frequencies in project. For example, for a random volume we have
np.allclose(vol.project(), manual_proj) == False, but
np.allclose(vol.project(zero_nyquist=False), manual_proj) == True,

where manual_proj is the manual projection of the volume found by taking the mean over the z-axis.

We can see these artifacts in the one_hot example I posted in the related issue, which now looks like this:

Ultimately, I think we just need to decide what we want our defaults to be.

[janden]

Looks great! Just one small question.

Regarding the artifacts, I think this is fine for now. Recall that the one-hot volumes and images are actually quite different from the type of images we expect to see (i.e., they are not smooth, but “discontinuous”).