Skip to content

Unit overhaul 3/6: Store Q as a scipp Variable in 1/angstrom#223

Open
henrikjacobsenfys wants to merge 1 commit into
split/02a-verify-q-indexfrom
split/02b-q-variable
Open

Unit overhaul 3/6: Store Q as a scipp Variable in 1/angstrom#223
henrikjacobsenfys wants to merge 1 commit into
split/02a-verify-q-indexfrom
split/02b-q-variable

Conversation

@henrikjacobsenfys

@henrikjacobsenfys henrikjacobsenfys commented Jul 6, 2026

Copy link
Copy Markdown
Member

Change _validate_and_convert_Q to return a scipp Variable (dimension 'Q', unit 1/angstrom) instead of a bare numpy array, and update the model hierarchy accordingly: ModelBase, InstrumentModel and DiffusionModelBase now expose Q as a Variable, compare Q with sc.allclose, and iterate via range(len(Q)); DeltaLorentz and the translational-diffusion models take .values where they need raw floats for naming and math.

This carries the canonical Q unit through the public .Q property, so scipp Q values in any convertible unit (e.g. 1/nm) are accepted and normalized. Tests updated to read Q via .values.

All unit tests pass.


Stack — splitting the monolithic "Unit system overhaul" (#217) into a reviewable series. Merge bottom-up:

  1. Add unit-conversion utilities and FitTarget → develop (Unit overhaul 1/6: Add unit-conversion utilities and FitTarget #221)
  2. Centralize Q-index validation via verify_Q_indexsplit/01-unit-utils (Unit overhaul 2/6: Centralize Q-index validation via verify_Q_index #222)
  3. Store Q as a scipp Variable in 1/angstromsplit/02a-verify-q-index (Unit overhaul 3/6: Store Q as a scipp Variable in 1/angstrom #223) (this PR)
  4. Separate model unit into x_unit/y_unit + update tutorials → split/02b-q-variable (Unit overhaul 4/6: Separate unit into x_unit/y_unit and update tutorials #224)

🤖 Generated with Claude Code

Change _validate_and_convert_Q to return a scipp Variable (dimension 'Q',
unit 1/angstrom) instead of a bare numpy array, and update the model
hierarchy accordingly: ModelBase, InstrumentModel and DiffusionModelBase
now expose Q as a Variable, compare Q with sc.allclose, and iterate via
range(len(Q)); DeltaLorentz and the translational-diffusion models take
.values where they need raw floats for naming and math.

This carries the canonical Q unit through the public .Q property, so scipp
Q values in any convertible unit (e.g. 1/nm) are accepted and normalized.
Tests updated to read Q via .values. Split out of the larger unit-system
branch.

All unit tests pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@henrikjacobsenfys henrikjacobsenfys added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority labels Jul 6, 2026
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.47%. Comparing base (a5ff526) to head (2379d58).

Additional details and impacted files

Impacted file tree graph

@@                    Coverage Diff                    @@
##           split/02a-verify-q-index     #223   +/-   ##
=========================================================
  Coverage                     97.47%   97.47%           
=========================================================
  Files                            53       53           
  Lines                          3685     3687    +2     
  Branches                        640      640           
=========================================================
+ Hits                           3592     3594    +2     
  Misses                           62       62           
  Partials                         31       31           
Flag Coverage Δ
unittests 97.47% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...iffusion_model/brownian_translational_diffusion.py 100.00% <100.00%> (ø)
...mics/sample_model/diffusion_model/delta_lorentz.py 97.58% <100.00%> (+<0.01%) ⬆️
...mple_model/diffusion_model/diffusion_model_base.py 96.87% <100.00%> (ø)
...el/diffusion_model/jump_translational_diffusion.py 100.00% <100.00%> (ø)
src/easydynamics/sample_model/instrument_model.py 100.00% <100.00%> (ø)
src/easydynamics/sample_model/model_base.py 97.56% <100.00%> (ø)
src/easydynamics/utils/utils.py 88.63% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrikjacobsenfys henrikjacobsenfys changed the title Unit overhaul 3/5: Store Q as a scipp Variable in 1/angstrom Unit overhaul 3/4: Store Q as a scipp Variable in 1/angstrom Jul 6, 2026
@henrikjacobsenfys henrikjacobsenfys changed the title Unit overhaul 3/4: Store Q as a scipp Variable in 1/angstrom Unit overhaul 3/6: Store Q as a scipp Variable in 1/angstrom Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[priority] medium Normal/default priority [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant