wavelength units should be angstrom #23

prjemian · 2020-09-02T22:16:17Z

fixes #17

The wavelength units must match the same units used by the lattice parameters. When X-ray energy is specified, the computed wavelength must be in angstrom.

prjemian · 2020-09-02T22:17:26Z

Note that @ambarb discussed this change previously as part of a different PR.

mrakitin

Looks good to me. I'd like to ask @ambarb to look at it as you are more closely dealing with this object.

mrakitin · 2020-09-03T20:26:39Z

The test failures are worrying. Were they passing before?

prjemian · 2020-09-03T21:43:35Z

I don't know. Need to look at them.

prjemian · 2020-09-25T17:04:23Z

The failures relate to tardis calculations of the forward solutions ((hkl) to motor positions). Might be affected by this code. Will apply tests on master branch to see if they are duplicate.

https://travis-ci.org/github/bluesky/hklpy/builds/723607153#L1494-L1506

prjemian · 2020-09-25T17:27:20Z

@mrakitin - I can't test this code since I do not have the example IOCs running. The error message offers some incorrect (assumed correct when it was written long ago) advice.

(hkl) prjemian@poof ~/.../hklpy/tests $ ./coverage_full.sh 
**ERROR** TThe Motor IOC is not running or is inaccessible from this network.  See ophyd.git/examples/iocs.

These tests assume the motor IOC example and the areaDetector IOC example are running.
PVs checked for:
  XF:31IDA-OP{Tbl-Ax:FakeMtr}-I (='')
  XF:31IDA-BI{Cam:Tbl}image1:Dim0SA (='')

This search may help to locate the current way to run: https://github.com/search?q=org%3Abluesky+examples%2Fiocs&type=code

Another point may be in how travis runs the test. Next place to look...

prjemian · 2020-09-25T17:36:51Z

Need to conda install pytest, then in root dir of repo: py.test tests will run the tests. Confirm same errors locally on this (17-angstrom-wavelength) branch.

Switching to master branch, run tests again (summary: no errors):

(hkl) prjemian@poof ~/.../projects/hklpy $ py.test tests
================================================== test session starts ===================================================
platform linux -- Python 3.7.7, pytest-6.0.2, py-1.9.0, pluggy-0.13.1
rootdir: /home/prjemian/Documents/projects/hklpy
collected 4 items                                                                                                        

tests/test_tardis.py ....                                                                                          [100%]

==================================================== warnings summary ====================================================
hkl/util.py:9
  /home/prjemian/Documents/projects/hklpy/hkl/util.py:9: PyGIWarning: Hkl was imported without specifying a version first. Use gi.require_version('Hkl', '5.0') before import to ensure that the right version gets loaded.
    from gi.repository import Hkl as hkl_module

tests/test_tardis.py::test_params
  /home/prjemian/Apps/anaconda/envs/hkl/lib/python3.7/site-packages/ophyd/utils/startup.py:10: UserWarning: This function is deprecated in ophyd 1.2 and will be removed in ophyd 1.3. setup is now automatically called.
    warnings.warn("This function is deprecated in ophyd 1.2 "

-- Docs: https://docs.pytest.org/en/stable/warnings.html
============================================= 4 passed, 2 warnings in 0.85s ==============================================

prjemian · 2020-09-25T17:37:18Z

Conclusion: I'll resolve those test errors in this PR.

prjemian · 2020-09-25T18:06:06Z

It should not be necessary to have EPICS PVs to test this code. It is possible to test this code with the ophyd.SoftPositioner. The switch SoftPositioner to is beyond the scope of this PR but should be the focus of a future issue.

prjemian · 2020-09-25T18:50:17Z

Confirmed that:

tardis tests use SoftPositioner already
tardis test conditions are specific to tardis.calc.wavelength = 1.3317319441460795
tardis test setup sets energy of 0.931 keV which calculates wavelength of 13.317 angstrom now

Easiest thing to do now is to set the test energy to 9.31 keV (even though that energy might be unrealistic for the NSLS-II tardis diffractometer but this code tests the calculation, not the NSLS-II instrument).

prjemian · 2020-09-25T19:07:39Z

@mrakitin Thought I'd ask you to review once more.
@ambarb: Could you also take a look at this?

I'd like to merge it by 2020-10-03 unless objections arise.

mrakitin

Looks good, have a couple of clarifications in the comments.

hkl/calc.py

tests/test_tardis.py

Co-authored-by: Maksim Rakitin <mrakitin@users.noreply.github.com>

ambarb · 2020-09-25T21:54:18Z

It should not be necessary to have EPICS PVs to test this code. It is possible to test this code with the ophyd.SoftPositioner. The switch SoftPositioner to is beyond the scope of this PR but should be the focus of a future issue.

I've used the bluesky simulator motors and had it working at CSX, but I didn't commit and someone deleted the file. It worked great as a replacement for spec onsim

mrakitin · 2020-09-25T21:55:29Z

@prjemian, I saw your lesson on hkl in https://github.com/BCDA-APS/use_bluesky/blob/master/lessons/lesson7.ipynb, it looks great!

ambarb · 2020-09-25T22:08:40Z

I don't see anything of concern. We should start working together though so we don't have competing higher level functions to interact with HKL. Some of the things in your lesson are very nice ( print formatting and simplify setting the calculation limits), but those are higher level functions.

ambarb · 2020-09-25T22:20:24Z

I also would say that a nice part of python that you may show off is something like, which is equivalent to some of the things I picked up from spec power users:

myhkl = tardis.position
myhkl_motors = tardis.real_positions` #or whatever is the actual attribute name

which is nice, because then you can move the diffractometer around and later return

tardis.forward(myhkl)
tardis.move(myhkl)

Or use it for simulating plans prior to running them without having to edit the plans - you cannot accurately simulate relative scans using print_summary, but you can fake it with absolute scans.

hh, kk, ll = myhkl
print_summary(scan([fccd, tardis], tardis.h, hh-.05, hh+.05, 21))

which in the end, is may be the way you want to write your plan so you are not needlessly thrashing around the motors to "return" to the starting point.

prjemian · 2020-09-25T23:34:10Z

Missed that detail. Thanks!

…

On Fri, Sep 25, 2020, 5:47 PM Maksim Rakitin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In hkl/calc.py <#23 (comment)>: > @@ -13,7 +13,9 @@ logger = logging.getLogger(__name__) +A_KEV = 12.39842 # 12.2984244 # angstrom * keV My point was that 12.2984244 was a wrong number, should be 12.*3*984244 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARMUMF3VA37P2CXDP3YQ23SHUMY7ANCNFSM4QT3Y2KA> .

fixes #17

02517f1

prjemian added the bug label Sep 2, 2020

prjemian requested a review from mrakitin September 2, 2020 22:16

prjemian self-assigned this Sep 2, 2020

prjemian added this to the 0.3.15 milestone Sep 2, 2020

mrakitin approved these changes Sep 3, 2020

View reviewed changes

prjemian added 4 commits September 25, 2020 13:53

MNT #23 ignore VSCode editor settings

f434144

MNT #23

c9fa2f0

TST #23 tests pass now

d2780d6

TST #23 tests pass LOCALLY with no warnings

08a9256

prjemian requested a review from mrakitin September 25, 2020 19:06

mrakitin reviewed Sep 25, 2020

View reviewed changes

hkl/calc.py Outdated Show resolved Hide resolved

tests/test_tardis.py Show resolved Hide resolved

tests/test_tardis.py Outdated Show resolved Hide resolved

accepted

993398c

Co-authored-by: Maksim Rakitin <mrakitin@users.noreply.github.com>

DOC #23 clarifications

0a74cbf

prjemian merged commit e5d0e8c into master Sep 26, 2020

prjemian deleted the 17-angstrom-wavelength branch September 26, 2020 14:23

This was referenced Sep 26, 2020

revise tardis unit test to use wavelength #27

Closed

add "wavelength" to Diffractometer class #29

Closed

consider units for wavelength, energy, & lattice parameters #30

Closed

prjemian mentioned this pull request Apr 21, 2021

re-arrange documentation structure #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wavelength units should be angstrom #23

wavelength units should be angstrom #23

prjemian commented Sep 2, 2020

prjemian commented Sep 2, 2020

mrakitin left a comment

mrakitin commented Sep 3, 2020

prjemian commented Sep 3, 2020

prjemian commented Sep 25, 2020 •

edited

Loading

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

mrakitin left a comment

ambarb commented Sep 25, 2020

mrakitin commented Sep 25, 2020 •

edited

Loading

ambarb commented Sep 25, 2020 •

edited

Loading

ambarb commented Sep 25, 2020

prjemian commented Sep 25, 2020 via email

wavelength units should be angstrom #23

wavelength units should be angstrom #23

Conversation

prjemian commented Sep 2, 2020

prjemian commented Sep 2, 2020

mrakitin left a comment

Choose a reason for hiding this comment

mrakitin commented Sep 3, 2020

prjemian commented Sep 3, 2020

prjemian commented Sep 25, 2020 • edited Loading

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

prjemian commented Sep 25, 2020

mrakitin left a comment

Choose a reason for hiding this comment

ambarb commented Sep 25, 2020

mrakitin commented Sep 25, 2020 • edited Loading

ambarb commented Sep 25, 2020 • edited Loading

ambarb commented Sep 25, 2020

prjemian commented Sep 25, 2020 via email

prjemian commented Sep 25, 2020 •

edited

Loading

mrakitin commented Sep 25, 2020 •

edited

Loading

ambarb commented Sep 25, 2020 •

edited

Loading