Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added d space unit in A #1970

Merged
merged 4 commits into from
Oct 9, 2023
Merged

Added d space unit in A #1970

merged 4 commits into from
Oct 9, 2023

Conversation

vallsv
Copy link
Collaborator

@vallsv vallsv commented Oct 5, 2023 •
edited
Loading

Feedback from ID06.

They expect to use this unit.

I am not quite sure on the description of the formula, but for sure the way to compute it is fine.

with:
    q in A^-1
    d in A
d = 2pi / q

For now i compute it in a dedicated notebook at ID06, but i will probably have to display it in Flint too, so i think it is better to share it at the same place, if possible.

@vallsv vallsv requested a review from kif October 5, 2023 12:17
@vallsv vallsv self-assigned this Oct 5, 2023
@vallsv vallsv added the ready to merge Please review label Oct 5, 2023
@gudlot
Copy link

gudlot commented Oct 5, 2023

It is an equation of how to convert q-values to d-spacing.

@vallsv
Copy link
Collaborator Author

vallsv commented Oct 5, 2023

It is an equation of how to convert q-values to d-spacing.

Yes, i think it is

@vallsv
Copy link
Collaborator Author

vallsv commented Oct 5, 2023

Reworked as requested to have ascale 1 with q in meter.

Notice that unit scale for q space, and eq_q() returns values in nm^-1 if i am not wrong.

Then i am kind of confuse

  • I am not sure if the compensation in the units module d_A.equation= is right.
  • But the test on unitutils.py (which dosn't use equation) is for sure fine

@kif
Copy link
Member

kif commented Oct 6, 2023

I will add a quick test that validates equation (implemented in numpy) and formula implemented in numexpr are actually equivalent...

@kif kif mentioned this pull request Oct 6, 2023
Merged
@kif
Copy link
Member

kif commented Oct 6, 2023 •
edited
Loading

The extra test in #1971 fails on this unit...

======================================================================
FAIL: test_all (pyFAI.test.test_units.TestUnits.test_all)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/users/kieffer/workspace-400/pyFAI/build/lib/python3.11/site-packages/pyFAI/test/test_units.py", line 66, in test_all
    self.assertTrue(numpy.allclose(ref,obt), f"Equation and formula do NOT match for {k}")
AssertionError: False is not true : Equation and formula do NOT match for d_A

@kif
Copy link
Member

kif commented Oct 6, 2023

I can submit you a patch ...

kif
kif approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@kif kif left a comment

Choose a reason for hiding this comment

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

LGTM

pyFAI/units.py Outdated
@@ -193,6 +193,7 @@ def eq_q(x, y, z, wavelength):
formula_2th = "arctan2(sqrt(x * x + y * y), z)"
formula_chi = "arctan2(y, x)"
formula_q = "4.0e-9*π/λ*sin(arctan2(sqrt(x * x + y * y), z)/2.0)"
formula_d = "λ*sin(arctan2(sqrt(x * x + y * y), z)/2.0)/2.0e-9"
Copy link
Member

Choose a reason for hiding this comment

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

There is a big difference between d* and d, the former is in reciprocal space, the later in real space. As a consequence d* ~ 1/d. This, this formula is wrong while the equation of d_A looks OK.

pyFAI/gui/utils/test/test_unitutils.py Outdated
@@ -50,6 +50,10 @@ def testFrom2ThRad__Q_A(self):
result = unitutils.from2ThRad(0.1, units.Q_A, wavelength=1.03321e-10)
self.assertAlmostEqual(result, 0.6078, places=3)

def testFrom2ThRad__RecD_A(self):
result = unitutils.from2ThRad(0.1, units.RecD_A, wavelength=1.03321e-10)
Copy link
Member

Choose a reason for hiding this comment

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

s/RecD_A/D_A/ see comment in units.py

pyFAI/units.py Outdated
@@ -390,6 +415,7 @@ def to_unit(obj, type_=None):
TTH_DEG = TTH = RADIAL_UNITS["2th_deg"]
R = R_MM = RADIAL_UNITS["r_mm"]
R_M = RADIAL_UNITS["r_m"]
RecD_A = RADIAL_UNITS["d_A"]
Copy link
Member

Choose a reason for hiding this comment

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

This should be D_A since the unit is in real space, not in Recreciprocal space. Would be nice to declare D_NM and D_M as well.

@kif kif merged commit 154585b into silx-kit:main Oct 9, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kif kif kif approved these changes

Assignees

@vallsv vallsv

Labels
ready to merge Please review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vallsv @gudlot @kif