Skip to content

Torch-native PDF pipeline + origin finder improvements - #248

Open
ehrhardtkm wants to merge 14 commits into
electronmicroscopy:devfrom
ehrhardtkm:pdf-torch
Open

ehrhardtkm wants to merge 14 commits into
electronmicroscopy:devfrom
ehrhardtkm:pdf-torch

Conversation

@ehrhardtkm

Copy link
Copy Markdown

What problem this PR addresses

Follow-on to #177 (the polar transform + PDF pipeline that merged into nanobeam). This PR brings that work onto dev and lands the changes I've made since.

Conceptually unchanged — same Polar4dstem datastructure, same PairDistributionFunction shape, same overall workflow (origin → polar → PDF). The public API should be the same, except for a few minor changes.

Changes since #177:

  • Fully torch-native, dual-backend pipeline. The whole pipeline accepts numpy or torch input on the existing Dataset, device is inferred from the input, and there are no GPU↔CPU round-trips inside the pipeline. numpy vs tensor input give identical results.

  • Background fitting. Rewritten with an analytic Jacobian that is noticeably faster than the previous implementation. Added a batched fit_bg_batched path for full-dataset use, which the downstream PDF widget (PR3) needs to recompute backgrounds interactively. Also includes some small fixes I picked up while running this on real data.

  • Origin finding. The existing finder's hardcoded margin requirement caused it to error out on smaller detectors, so I made it the margin dynamic and added a 2D-quadratic subpixel refinement (renamed to find_origin_angular_grid). I also added a secondary option, find_origin_angular_descent, a margin-free, COM-anchored descent finder. It's slower but I've found it slightly more reliable on noisier data. Both are dispatched from Polar4dstem.from_data(origin_method="grid"|"descent").

  • Tests in tests/diffraction/test_polar.py are parametrized over both origin finders and add coverage for the new code paths.

Out of scope (follow-on PRs to shortly follow):

  • Elliptical-distortion fit + reciprocal-pixel calibration → PR2, stacks on this branch.
  • ShowPDF widget vendor → PR3.

What should the reviewer(s) do

Merge after review. Reviewing commit-by-commit is likely easier than the unified diff — each commit is a single conceptual change.

  • This PR introduces a public-facing change (e.g., figures, CLI input/output, API).
    • For functional and algorithmic changes, tests are written or updated.
    • Documentation (e.g., tutorials, examples, README) has been updated.
    • A tracking issue or plan to update documentation exists.
  • This PR affects internal functionality only (no user-facing change).

@bobleesj

Copy link
Copy Markdown
Collaborator

@ehrhardtkm before I review, it seems like being merged to upstream/dev instead of upstream/nanobeam. Has upstream/nanobeam containing pdf code merged to upstream/dev?

@bobleesj

bobleesj commented Jun 12, 2026 •

Copy link
Copy Markdown
Collaborator

Another question I had was from the PR comment was - what's approx speed/improvement given various sizes? Mostly likely it will be either 512x512x192x192 or 256x256x192x192 to be practical, just ballpark numbers.

Ref:

@ehrhardtkm

Copy link
Copy Markdown
Author

@ehrhardtkm before I review, it seems like being merged to upstream/dev instead of upstream/nanobeam. Has upstream/nanobeam containing pdf code merged to upstream/dev?

@bobleesj upstream/nanobeam hasn't been merged into upstream/dev and no PDF code exists on dev yet. The issue with merging nanobeam into dev is that nanobeam has diverged significantly from dev (50 commits ahead and over 400 commits behind` and including other new modules (MAPED, strain mapping) that aren't all fully torch-native.

This PR carries just the PDF code from PR #177 plus improvements/fixes since then, which are listed in this PR's description. If nanobeam ever gets merged into dev, we should take dev's version of the PDF files.

@bobleesj

Copy link
Copy Markdown
Collaborator

@ehrhardtkm ah-ha divergence! Often inevitable esp for scientific code - maybe before the hackathon, if possible, we shall have nanobeam merged to dev. What do you think? MAPED, strain, PDF, these are super important for the upcoming.

@ehrhardtkm

Copy link
Copy Markdown
Author

@bobleesj I don’t think nanobeam can go into dev as a plain merge. Apart from the PDF code it carries 5k lines (MAPED, strain mapping, a file reader, imaging_utils changes). Also, from what I see, its imaging_utils fixes the same sub-pixel cross-correlation bug as dev’s #281, plus open PRs #284 and #97 are also on newer version of the strain code. It would be cleaner for at least the strain and PDF code to go into dev in these separate merges, I think.

@bobleesj
bobleesj self-requested a review October 15, 2026 07:51
@bobleesj

Copy link
Copy Markdown
Collaborator

@bobleesj I don’t think nanobeam can go into dev as a plain merge.

happy to merge this PR to dev and we can incrementally port things over to dev - it doesn't have to all come from nanobeam into dev. Coding is landscape is changing.

@arthurmccray could you please check if I have permission for merging?

Screenshot 2026-10-15 at 12 51 42 AM

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.

2 participants