Skip to content

Spectral curve and Lorentz correction support#195

Open
jglaser wants to merge 19 commits intors-station:mainfrom
jglaser:spectral_curve
Open

Spectral curve and Lorentz correction support#195
jglaser wants to merge 19 commits intors-station:mainfrom
jglaser:spectral_curve

Conversation

@jglaser
Copy link

@jglaser jglaser commented Dec 1, 2025

This PR is a proof-of-concept for how to integrate physics-based corrections as a deterministic scaling function, allowing for a single trainable, and multiplicative intensity scale. See issue #194 for a discussion of the conceptual dissonance between this physics-based approach and the standard data-driven one. We should refine this PR as necessary.

Example usage:

Apply the wavelength-dependent instrument correction, stored in a two-column text file called spectral_curve.txt, and the analytical Lorentz correction, during harmonic deconvolution:

careless poly \
  --iterations=30_000 \
  --merge-half-datasets \
  --separate-files \
  --wavelength-key='WAVEL' \
  --spectral-file spectral_curve.txt \
  --lorentz-correction \
  --trainable-spectral-scale \
  --wavelength-range 2 4 \
  "WAVEL" \
  uncorrected_data_in_asu.mtz \
  my_output_prefix

@jglaser
Copy link
Author

jglaser commented Dec 2, 2025

This PR should be ready for review and CI

Copy link
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for the most part. I highlighted some places where the documentation could be improved (but it is pretty good already). I asked for two small refactors to your Scaler.

Finally, please figure out whether your Scaler is compatible with --scale-file (see test). If so, let's add an explicit test for it. Otherwise, we should warn users that it isn't supported. With at most one trainable variable, my suspicion is it won't be a heavily a used feature, but I'd just like to make sure.

@kmdalton
Copy link
Member

kmdalton commented Dec 9, 2025

hi @jglaser -- just checking in. let me know if you need any help with this or if my review wasn't clear.

@jglaser
Copy link
Author

jglaser commented Dec 16, 2025

Finally, please figure out whether your Scaler is compatible with --scale-file (see test). If so, let's add an explicit test for it. Otherwise, we should warn users that it isn't supported. With at most one trainable variable, my suspicion is it won't be a heavily a used feature, but I'd just like to make sure.

done

@jglaser
Copy link
Author

jglaser commented Dec 21, 2025

hi @jglaser -- just checking in. let me know if you need any help with this or if my review wasn't clear.

Hi @kmdalton .... thank you for your detailed review. I applied the changes (after successfully returning from vacation :). The PR should be complete now.

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