1) Control frequency and starting epoch when we update the probe, 2) control frequency and whether we update sample/probe scaling using mean of abs of object

a4894z commented 11 months ago

Purpose

Added some parameters under probe and object options which:

1) Control when we start probe updates and the frequency (wrt epoch) of updating 2) Control when (wrt epoch) we rescale the object and probe using mean of object magnitude

Approach

I've added some new parameters under probe options as well as object options which perform the following:

update_probe """Boolean switch used to indicate whether to update probe or not."""

update_start """Start probe updates at this epoch."""

update_skip """Only update probe when mod( current epoch, update_skip ) == 0"""

init_rescale_from_measurements """Initial rescaling of probe using measured intensity."""

rescale_using_mean_of_abs_object """ Rescale sample (and probe) so that average sample magnitude is around 1.0 """

rescale_using_mean_of_abs_object_skip """ Frequency (wrt epoch) we rescale sample/probe so that average sample magnitude is around 1.0 """

From these, I then just change the if statements in the code to update the probe or rescale the probe and object according to our choices above.

Pre-Merge Checklists

Submitter

[ ] Write a helpfully descriptive pull request title.
[ ] Organize changes into logically grouped commits with descriptive commit messages.
[ ] Document all new functions.
[ ] Click 'details' on the readthedocs check to view the updated docs.
[ ] Write tests for new functions or explain why they are not needed.
[ ] Address any complaints from pep8speaks.

Reviewer

[ ] Actually read all of the code.
[ ] Run the new code yourself; the included tests should make this easy.
[ ] Write a summary of the changes as you understand them.
[ ] Thank the submitter.

pep8speaks commented 11 months ago

Hello @a4894z! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file src/tike/ptycho/probe.py:

Line 922:81: E501 line too long (87 > 80 characters) Line 923:67: E128 continuation line under-indented for visual indent

In the file src/tike/ptycho/ptycho.py:

Line 427:81: E501 line too long (89 > 80 characters) Line 505:81: E501 line too long (92 > 80 characters) Line 507:81: E501 line too long (120 > 80 characters) Line 522:81: E501 line too long (100 > 80 characters) Line 523:81: E501 line too long (124 > 80 characters) Line 538:17: E262 inline comment should start with '# '

In the file src/tike/ptycho/solvers/options.py:

Line 49:81: E501 line too long (84 > 80 characters)

Comment last updated at 2023-12-04 16:51:36 UTC

carterbox commented 11 months ago

Some of the tests are failing now because PtychoParameters.probe_options was previously allowed to be None, but you don't always check if it is None before accesssing PtychoParameters.probe_options.recover_probe. Is it confusing that PtychoParameters.probe_options can be None? Should we force the user to always provide a ProbeOptions? Then to disable probe options you would have to set ProbeOptions.update_start = np.inf; this is logical, but maybe not intuitive.

a4894z commented 11 months ago

Some of the tests are failing now because PtychoParameters.probe_options was previously allowed to be None, but you don't always check if it is None before accesssing PtychoParameters.probe_options.recover_probe. Is it confusing that PtychoParameters.probe_options can be None? Should we force the user to always provide a ProbeOptions? Then to disable probe options you would have to set ProbeOptions.update_start = np.inf; this is logical, but maybe not intuitive.

The other option would be to have an "if statement" which checks if PtychoParameters.probe_options is None, and then if it gets passed that, then checks for e.g. PtychoParameters.probe_options.init_rescale_from_measurements or PtychoParameters.probe_options.recover_probe?

carterbox commented 11 months ago

The other option would be to have an "if statement" which checks if PtychoParameters.probe_options is None, and then if it gets passed that, then checks for e.g. PtychoParameters.probe_options.init_rescale_from_measurements or PtychoParameters.probe_options.recover_probe?

Yes, that is what is currently done in a few places in the code already.

a4894z commented 11 months ago

The other option would be to have an "if statement" which checks if PtychoParameters.probe_options is None, and then if it gets passed that, then checks for e.g. PtychoParameters.probe_options.init_rescale_from_measurements or PtychoParameters.probe_options.recover_probe?

Yes, that is what is currently done in a few places in the code already.

OK, try now?

a4894z commented 11 months ago

Instead of introducing the new parameter "rescale_using_mean_of_abs_object_period" then removing it later. Just skip to implementing "rescale_method", but only offer one option. That way, there are fewer API breaking changes for Steve to adapt to.

It's a little awkward since calling remove_object_ambiguity using the rescale_using_mean_of_abs_object_period switch rescales both the object and probe (and is included in object.py) while the alternate method I'll introduce rescale_probe_using_fixed_intensity_photons rescales just the probe (and is included in probe.py).

I agree it'll be better if we just have a rescale_method switch which defaults to rescale_using_mean_of_abs_object_period, but I'm not sure where to put it (object.py or probe.py)?

Or should it default to only rescaling the probe using the diffraction measurements as the fixed number of photons?

How about putting rescale_method in algorithm_options?

Also, why can't difference map use the remove_object_ambiguity function?

There's no object/probe scaling control when using difference map?

a4894z commented 11 months ago

How about keeping the condition for getting to remove_object_ambiguity in place, and have a default "else" case which just rescales the probe using a fixed photon intensity...that fixed photon intensity will be either in the toml file or defaults to using the measured diffraction?

carterbox commented 11 months ago

It's a little awkward since calling remove_object_ambiguity using the rescale_using_mean_of_abs_object_period switch rescales both the object and probe (and is included in object.py) while the alternate method I'll introduce rescale_probe_using_fixed_intensity_photons rescales just the probe (and is included in probe.py).

This is a good point! I agree one has a strong argument to say that resolving the object/probe scaling ambiguity is both/neither a probe and object correction.

How about putting rescale_method in algorithm_options?

Sounds good.

Also, why can't difference map use the remove_object_ambiguity function? There's no object/probe scaling control when using difference map?

I think that I disabled rescaling for DM because it wasn't part of the original paper and it didn't work well. I didn't want complaints about this feature being broken for DM, so I just disabled it.

a4894z commented 11 months ago

It's a little awkward since calling remove_object_ambiguity using the rescale_using_mean_of_abs_object_period switch rescales both the object and probe (and is included in object.py) while the alternate method I'll introduce rescale_probe_using_fixed_intensity_photons rescales just the probe (and is included in probe.py).

This is a good point! I agree one has a strong argument to say that resolving the object/probe scaling ambiguity is both/neither a probe and object correction.

How about putting rescale_method in algorithm_options?

Sounds good.

Also, why can't difference map use the remove_object_ambiguity function? There's no object/probe scaling control when using difference map?

I think that I disabled rescaling for DM because it wasn't part of the original paper and it didn't work well. I didn't want complaints about this feature being broken for DM, so I just disabled it.

have a look now?

carterbox commented 11 months ago

Thanks @a4894z for implementing this feature so quickly!

AdvancedPhotonSource / tike