Repository navigation
Switch to a more sensible normalization strategy for loss metrics - #70
Merged
Merged
Conversation
…y which allows for a nice normalization of the Poisson NLL. The slow tests will now likely fail
…malizations, and add the new normalization to all models
Adds 'intensity_mse' as a selectable loss function (alongside the existing 'amplitude_mse' and 'poisson_nll') to FancyPtycho, MultislicePtycho, Bragg2DPtycho, Multislice2DPtycho, and RPI. Models that previously had a hardcoded amplitude_mse assignment now use the same configurable pattern as FancyPtycho, with the loss parameter threaded through from_dataset and from_calibration as well. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Updates tutorial_simple_ptycho.py to match simple_ptycho.py: removes the def loss() method and instead assigns self.loss and self.loss_normalizer as instance attributes in __init__. Updates tutorial.rst accordingly: adds the loss assignment to the __init__ code block with an explanation, and removes def loss() from the forward model section. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
poisson_nll now returns a sum rather than a mean, consistent with the normalizer pattern. Remove the per-pixel divisions from the numpy reference calculations accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use poisson_nll in test_near_field_ptycho and intensity_mse in test_Adam_gold_balls to exercise these loss paths end-to-end. Thresholds left as-is pending re-running on a GPU machine. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Revert test_Adam_gold_balls to default amplitude_mse - Add new test_intensity_MSE test (gold balls + AdamReconstructor) - Update near_field threshold to 17 (poisson_nll scale) Thresholds for test_intensity_MSE to be tuned after GPU run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aved as a numpy scalar, and added test coverage
allevitan
commented
Apr 13, 2026
allevitan
left a comment
allevitan
left a comment
Collaborator
Author
There was a problem hiding this comment.
Make the fixes, then okay to merge
|
|
||
| 2. **Normalization scaling**: Divides by 0.5 times the count of non-zero | ||
| pixels in the measured patterns. This is because, roughly, each non-zero | ||
| pixel is expected to contribute to the Poisson NLL, if Poisson noise were |
Collaborator
Author
There was a problem hiding this comment.
should be "...contribute 0.5 to..."
| import torch as t | ||
| import numpy as np | ||
| import pickle | ||
| from matplotlib import pyplot as plt |
Collaborator
Author
There was a problem hiding this comment.
Why was this deleted? Isn't it needed?
… currently, the intensity MSE loss was by default a mean, but amplitude was by default a sum. Both functions now have a flag, use_sum=False, which is False by default. This changes the default behavior of amplitude_mse. All models, including the tutorial version of simple_ptycho, are updated accordingly and test coverage was added.
…g with SimplePtycho, and update the thresholds to accomodate the new normalization with masks and for intensity_MSE
allevitan
commented
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes loss metrics a bit more interpretable. I may put some more work into writing better normalizations if I have a similar internet-free train ride in the future, where I can do some cdtools work but don't have a stable connection to any GPUS...
Key notes:
Changes: