BCDA-APS / bluesky_training

Bluesky training, including instrument package
https://bcda-aps.github.io/bluesky_training/
Other
11 stars 0 forks source link

DOC: scan area detector with motor(s) #311

Closed prjemian closed 1 week ago

prjemian commented 2 weeks ago

@sureshnaps, @qzhang - Can you examine this notebook? It can guide you during commissioning activities.

Start here: https://bcda-aps.github.io/bluesky_training/howto/_ad_v_motors.html#The-(1-D)-bp.scan()-plan-with-one-motor

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9520419244

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
rodolakis commented 2 weeks ago

In general, I find some of the markdown comment difficult to find in the middle of the code cell output, like below; maybe adding bullet point or a <br> before the comment would help.

image
rodolakis commented 2 weeks ago

Not sure what that bug was in the last few suggestions. It was displaying the correct sentence before pressing the green button, but then showed the code for an image as the original...

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523403052

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523404999

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523406656

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523409301

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523411126

Details


Totals Coverage Status
Change from base Build 9506811548: 0.08%
Covered Lines: 1021
Relevant Lines: 1226

💛 - Coveralls
github-actions[bot] commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523460257

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523460257

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
github-actions[bot] commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523476197

Details


Totals Coverage Status
Change from base Build 9506811548: 0.08%
Covered Lines: 1021
Relevant Lines: 1226

💛 - Coveralls
github-actions[bot] commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523476197

Details


Totals Coverage Status
Change from base Build 9506811548: 0.08%
Covered Lines: 1021
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523476197

Details


Totals Coverage Status
Change from base Build 9506811548: 0.08%
Covered Lines: 1021
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523488624

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523492504

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
github-actions[bot] commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523501661

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523501661

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523506916

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523514398

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523539610

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
prjemian commented 2 weeks ago

@rodolakis I believe this is ready for review again.

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523682643

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9523716939

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
rodolakis commented 2 weeks ago

OK so this is what I was trying to suggest:

image

It looks good in preview, but when I click "add review comment" it becomes wrong (quoting the giant image code instead).

rodolakis commented 2 weeks ago

In general, for all 3 sections, I would replace the sentence:

Step scan a motor (m1) and measure 3 (as configured above) images at each step. Also record total counts and the maximum counts value for each step.

With a bullet point sentence:

rodolakis commented 2 weeks ago
image

I would write instead:

Both motors, m1 and m2, move at the same time. Below is a plot showing the readback values of each motor during a two-motor scan versus the data point number:

rodolakis commented 2 weeks ago

The file name, as the catalog will find its image contents, is reconstructed:

I am not sure I understand this sentence. I was proposing:

"The catalog uses the file name to locate and retrieve its image contents. This file name is reconstructed as follows:"

rodolakis commented 2 weeks ago

"The second motor of the grid_scan (m2) is snaked. It's direction changes with each subsequent increment of the first motor (m1). Here's a plot of readback value of each motor from a grid_scan plotted v. data point number:\n",

I was proposing:

During the grid_scan, the second motor (m2) moves in a snaking pattern: its direction changes with each subsequent increment of the first motor (m1). Below is a plot showing the readback values of each motor during a grid_scan versus the data point number:

rodolakis commented 2 weeks ago
image
rodolakis commented 2 weeks ago

@prjemian I rewrote my broken comments. I still cannot quote anything as a github "suggestions" past a certain point in the document (only works at the beginning of the notebook for some reason)

prjemian commented 2 weeks ago

@rodolakis : I believe I have responded to all your comments. Thanks for the thorough review.

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9530038078

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9530038078

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9530038078

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
rodolakis commented 1 week ago

All good. Everything has been resolved as far as I am concerned except for one minor formatting suggestion (I tagged you on it). So minor that I'm fine with merging with or without.

prjemian commented 1 week ago

@rodolakis Thanks!

coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 9589569745

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 9589569745

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls
coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 9589569745

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Totals Coverage Status
Change from base Build 9506811548: 0.0%
Covered Lines: 1020
Relevant Lines: 1226

💛 - Coveralls