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

@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 98.02%. Comparing base (c33a479) to head (5391f75).

Additional details and impacted files

Impacted file tree graph

@@                    Coverage Diff                    @@
##           split/02a-verify-q-index     #223   +/-   ##
=========================================================
  Coverage                     98.01%   98.02%           
=========================================================
  Files                            53       53           
  Lines                          3685     3687    +2     
  Branches                        640      640           
=========================================================
+ Hits                           3612     3614    +2     
  Misses                           43       43           
  Partials                         30       30           
Flag Coverage Δ
unittests 98.02% <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 98.86% <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
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 force-pushed the split/02a-verify-q-index branch from a5ff526 to c33a479 Compare July 8, 2026 08:52
@AndrewSazonov
AndrewSazonov self-requested a review July 21, 2026 06:10

@AndrewSazonov AndrewSazonov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Non-blocking: it looks like Q now has two different meanings in different parts of the code: sometimes it is a unit-aware scipp.Variable, and sometimes it is a raw NumPy array of values. Assignments like Q = Q.values make that switch less obvious. Consider using a separate name, for example q_values = Q.values, in places where the code intentionally drops the unit and works with raw values.

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.

2 participants