-
Notifications
You must be signed in to change notification settings - Fork 4
the old dict learning code was incompatible with multislice and OPRs, this should fix it (for Nopr = 1), for Nopr>1, not sure #36
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
Conversation
this should fix it (for Nopr = 1), for Nopr>1, not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR rewrites the dictionary learning update rules to support multislice objects and OPRs (for a single probe), and refactors sparse-code–based probe initialization and generation.
- Reimplemented
compute_updatesin the PIE reconstructor to handle multiple slices and integrate sparse code updates. - Refactored probe data structure to derive and register sparse-code–based weights instead of initial weights.
- Updated step‐weight calculations and tensor assignments for sparse‐dictionary probes.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ptychi/reconstructors/pie.py | Overhauled compute_updates for multislice and sparse‐code probe updates; added slice loops |
| src/ptychi/data_structures/probe.py | Renamed get_initial_weights → get_sparse_code_weights; revised generate tensor handling |
Comments suppressed due to low confidence (1)
src/ptychi/reconstructors/pie.py:95
- [nitpick] The variable
unique_probesnow holds data for all slices rather than a single slice; consider renaming it toshifted_probes_per_sliceor similar for clarity.
unique_probes = self.forward_model.intermediate_variables.shifted_unique_probes
src/ptychi/reconstructors/pie.py
Outdated
| #from PySide6.QtWidgets import QApplication | ||
| import numpy as np | ||
| import matplotlib.pyplot as plt | ||
| import matplotlib as mpl | ||
| from mpl_toolkits.axes_grid1 import make_axes_locatable | ||
| #mpl.use('Agg') | ||
| #mpl.use('TKAgg') | ||
| #mpl.use('WebAgg') | ||
| mpl.use('QTAgg') | ||
| mpl.rcParams['agg.path.chunksize'] = 10000 |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move heavy imports and plotting code out of the inner update loop; consider extracting debug visualization into a separate utility or removing before merge.
| #from PySide6.QtWidgets import QApplication | |
| import numpy as np | |
| import matplotlib.pyplot as plt | |
| import matplotlib as mpl | |
| from mpl_toolkits.axes_grid1 import make_axes_locatable | |
| #mpl.use('Agg') | |
| #mpl.use('TKAgg') | |
| #mpl.use('WebAgg') | |
| mpl.use('QTAgg') | |
| mpl.rcParams['agg.path.chunksize'] = 10000 | |
| # Debug visualization (if needed) | |
| self._debug_visualize(sparse_code_probe) |
src/ptychi/data_structures/probe.py
Outdated
| # is this the correct way to retain the originally initialized grad? | ||
| self.tensor.data.data = torch.stack([probe.real, probe.imag], dim=-1) |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly manipulating .data is deprecated. Use APIs like with torch.no_grad(): self.tensor.copy_(...) or self.tensor.data = probe.view_as(self.tensor) to preserve autograd correctness.
| # is this the correct way to retain the originally initialized grad? | |
| self.tensor.data.data = torch.stack([probe.real, probe.imag], dim=-1) | |
| # Safely update self.tensor without breaking autograd | |
| with torch.no_grad(): | |
| self.tensor.copy_(torch.stack([probe.real, probe.imag], dim=-1)) |
before I was trying it for the OPRs...this is not necessary since we can just rely on the power method updates for the OPRs
…ty-chi into multisliceRPIE_v2
5add0c1 to
e838253
Compare
Features/fixes
The previous attempt at merging dict learn and multislice was hopeless, so I've rewritten the dict learn update rules to be compatible with multislice and OPRs
Related issues (optional)
See the previous pull request "#34"
Mentions
Ming Du
Checklist
Have you...
Formatted your code properly adhering to PEP-8? Considering using RUFF to format your code automatically.
doubt it
Resolved all merge conflicts with the main branch?
not sure yet
I think so