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

Use phase data in main workflow and add option for phase noise map #10

Merged
merged 9 commits into from
Jan 2, 2025

Conversation

tsalo
Copy link
Contributor

@tsalo tsalo commented Oct 3, 2024

Closes #8.

Changes proposed:

  • Fix a bug in the main workflow where phase data were not loaded.
  • Add a new parameter, --noise-map-phase, for the phase component of the noise map.
    • I'm not actually sure what rescaling should be done before this map is passed along to patch-denoise, since the phase data need to be in radians ahead of time.
  • Fix affine checks. Before it would raise a warning if the affines did match.

@tsalo
Copy link
Contributor Author

tsalo commented Oct 3, 2024

I'm going to put this in draft mode until I know what the scaling situation should be for the phase noise volumes.

@tsalo tsalo marked this pull request as draft October 3, 2024 19:14
@paquiteau
Copy link
Owner

Ah I remember having the same issue. If no phase unwrapping has be done, it's hard. The header does not necessarily contains the information (even in the DICOM, I remember having to look into a siemens raw data .dat file), and it's probably vendor specific.

A layman solution would be to let the user provide this to us.

@tsalo
Copy link
Contributor Author

tsalo commented Oct 4, 2024

I didn't think NORDIC_Raw unwrapped the phase data.

The header does not necessarily contains the information (even in the DICOM, I remember having to look into a siemens raw data .dat file)

Sorry, which information? The scale of the phase data?

What about rescaling the phase data and noise data in one array, then splitting it up? Not within patch-denoise of course, since the data are expected to be in radians already.

Comment on lines 80 to 93
print(f"Seed: {seed}")
rng = np.random.RandomState(seed)
medium_random_matrix = rng.randn(200, 200, 100)
print(f"Mean of raw: {np.nanmean(medium_random_matrix)}")
print(f"Max of raw: {np.nanmax(medium_random_matrix)}")
print(f"Min of raw: {np.nanmin(medium_random_matrix)}")

noise_map = estimate_noise(medium_random_matrix, block_dim)
print(f"Mean of noise map: {np.nanmean(noise_map)}")
real_std = np.nanstd(medium_random_matrix)
print(f"SD of raw: {real_std}")
err = np.nanmean(noise_map - real_std)
print(f"Err: {err}")
assert err <= 0.1 * real_std
Copy link
Contributor Author

@tsalo tsalo Oct 4, 2024

Choose a reason for hiding this comment

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

I'm digging into the intermittent test failures with the noise maps. The noise map sometimes has a really massive value. I'm not sure why.

Seed: 0
Mean of raw: 0.0004815311392502157
Max of raw: 4.7965495404996
Min of raw: -5.031764123854618
SD of raw: 0.9994477899078287
Mean of noise map: 134637028848.5359
Max of noise map: 527766886400.0
Min of noise map: 5e-323
SD of noise map: 2550814615827.277
Err: 134637028847.53642

Copy link
Contributor Author

@tsalo tsalo Oct 4, 2024

Choose a reason for hiding this comment

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

If the thermal noise volumes have values from -5 to 5, I can't see any way for the noise map to have values from 0 to 527766886400. I think there's a bug in the noise map estimation.

@paquiteau WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't reproduce the error locally. I've tried with seeds going from 0 to 60 and block sizes from 5 to 10, but never get these massive noise map values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking it must be something to do with the float precision.

@tsalo
Copy link
Contributor Author

tsalo commented Dec 31, 2024

We just need to concatenate the raw phase and noise phase, then rescale, then split.

@tsalo tsalo marked this pull request as ready for review December 31, 2024 15:42
@paquiteau paquiteau self-requested a review December 31, 2024 18:02
Copy link
Owner

@paquiteau paquiteau left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks for all the fixes! There stil seem sto be some flakky tests. I will look into next year ;)

@tsalo
Copy link
Contributor Author

tsalo commented Jan 2, 2025

If you're okay merging then please do. I think the test errors can be addressed in a separate PR.

@paquiteau paquiteau merged commit 8d8bd61 into paquiteau:master Jan 2, 2025
3 of 7 checks passed
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.

Phase data seem to be ignored in CLI
2 participants