Conversation
allevitan
left a comment
allevitan
left a comment
There was a problem hiding this comment.
Overall, these changes are small and good, but unfortunately they have revealed some larger pre-existing issues which we should fix before merging it! The issues are:
- The "gold_ball_synthesize.py" example function, which is the main example showing how to use the
standard_reconstruction_setfunction, is not working at present. - There are a few silent and non-silent bugs in the
standardidze_reconstruction_setandstandardize_reconstruction_pairfunctions. They have to do primarily with the object and probe basis not being necessarily equal anymore - There is no test coverage for the
standardidze_reconstruction_setandstandardize_reconstruction_pairfunctions. This is how the functions got so messed up in the first place!
I think these issues are pretty high-priority and merit a bit of work ASAP. I will try find time to send in some commits on the check_analysis_tools branch to fix these larger issues.
Finally, I put a few smaller comments on specific things to call out for the PR.
| correct_amplitude_exponent=False, | ||
| window=np.s_[:,:], | ||
| window = np.s_[:,:], | ||
| window_frc = np.s_[:,:], |
There was a problem hiding this comment.
This will change the default behavior, i.e. if there was a script which set "window" before, they'd be assuming that this also sets what is now window_frc, but the new behavior requires both to be set.
I suggest we set "window_frc=None", and then in the set it equal to window if it is not set explicitly.
There was a problem hiding this comment.
Also, maybe "frc_window" will be clearer than "window_frc", just because it is read out as a more natural phrase, e.g. "let's change the FRC window" vs "let's change the window FRC"?
| obj_1, window, probe=probe_1, | ||
| weights=half_1['weights'], | ||
| basis=half_1['basis'], | ||
| basis=half_1['obj_basis'], |
There was a problem hiding this comment.
Uh oh, so this does stop this code from failing, but it introduces a silent bug. With the latest versions of fancy_ptycho, obj_basis is not necessarily equal to probe_basis. remove_amplitude_exponent assumes they are equal.
So, the behavior before this PR is to simply fail, which is not good, but we should fix remove_amplitude_exponent before merging. Also, I suspect there is an equivalent silent bug in remove_phase_ramp at present, which we also need to fix 😬
| t.as_tensor(obj_2), shift_2).numpy() | ||
|
|
||
| freqs, frc, threshold = calc_frc( | ||
| ip.hann_window(obj_1[window]), |
There was a problem hiding this comment.
This has nothing to do with your code, but at present this apodization is a bit nonstandard - while we're mucking around with this code, I may push a commit to this section to update the apodization strategy to match e.g. ptychoshelves
2 participants
Modifications / corrections of some analysis tools used to standardize reconstructions.
Main change:
in standardize_reconstruction_set(), we sometime want to use a different window for the fitting to remove phase ramp/amplitude exponent than to calculate the FRC, therefore I added a 'window_FRC' argument.
The rest is few tweaks and corrections here and there.