MXDGA-3724: Added the MVP for the fast grid scan motion #2

DominicOram · 2022-01-04T19:49:29Z

See https://jira.diamond.ac.uk/browse/MXDGA-3724

To review:

Confirm that the Fast Grid Scan device is doing everything required for https://jira.diamond.ac.uk/browse/MXGDA-3733
Confirm unit tests run
Note that the system tests will not work against s03 currently as the fast grid scan IOC is not running on it

graeme-winter

Made a couple of comments, but these are just opinions on scanning the code.

Do we use black for formatting?

I don't know what the "house style" is for blue sky so I don't know if set_params(1, 2, 3, 4, 5) is good practice or no...

graeme-winter · 2022-01-06T07:17:12Z

src/artemis/devices/unit_tests/test_gridscan.py

+from ophyd.sim import make_fake_device
+from src.artemis.devices.fast_grid_scan import FastGridScan, time
+
+from mockito import *


Personally I prefer to import what you use rather than from thing import * - so for example when I wonder what when() is below, I can see where it comes from

Agreed, I wouldn't use import * in the actual production code so I shouldn't in the test code.

graeme-winter · 2022-01-06T07:20:29Z

src/artemis/devices/fast_grid_scan.py

+    # Kickoff timeout in seconds
+    KICKOFF_TIMEOUT: float = 5.0
+
+    def set_program_data(self, nx, ny, width, height, exptime, startx, starty, startz):


Kinda feel like more structured API here would help? set_program_data call in the test just looks like pushing a bunch of random numbers

Yh, I left this in as a bit of a placeholder until I had a better feel for how we would actually be using the Device upstream but I'll think about improving it now.

I borrowed some of the work of @callumforrester for this, which also included some checking against motor limits.

Didn't spot this chain, I think this needs updating to reflect this thread: #2 (comment)

DominicOram · 2022-01-06T13:26:17Z

Made a couple of comments, but these are just opinions on scanning the code.

Do we use black for formatting?

I don't know what the "house style" is for blue sky so I don't know if set_params(1, 2, 3, 4, 5) is good practice or no...

I've been using black, have a mild preference for it as it avoids arguments over specific formatting settings. Not sure if ophyd/bluesky use it. Looks like they use flake8 (https://github.com/bluesky/ophyd/blob/master/.flake8) but they're not necessarily mutually exclusive. Now that this repo is starting to grow I'll add a ticket in for some CI, including format checking against a similar flake8 profile to them.

… motor limits

graeme-winter · 2022-01-07T07:57:33Z

Made a couple of comments, but these are just opinions on scanning the code.
Do we use black for formatting?
I don't know what the "house style" is for blue sky so I don't know if set_params(1, 2, 3, 4, 5) is good practice or no...

I've been using black, have a mild preference for it as it avoids arguments over specific formatting settings. Not sure if ophyd/bluesky use it. Looks like they use flake8 (https://github.com/bluesky/ophyd/blob/master/.flake8) but they're not necessarily mutually exclusive. Now that this repo is starting to grow I'll add a ticket in for some CI, including format checking against a similar flake8 profile to them.

We already have black and flake8 set up as pre-commit hooks in most of our analysis code, so we can probably just copypasta the configuration over. Prefer pre-commit to working it out in a CI system as keeps the history cleaner... and makes for easier to read diffs.

callumforrester

Looks great, Dom. Code is very clear and well-documented. I particularly like the studious use of type annotations! Have added a few minor comments for things I spotted here and there.

src/artemis/devices/fast_grid_scan.py

src/artemis/devices/motors.py

src/artemis/devices/fast_grid_scan.py

callumforrester · 2022-01-07T09:07:24Z

src/artemis/devices/fast_grid_scan.py

+        except ZeroDivisionError:
+            fraction = 1
+            time_remaining = 0
+        except Exception:


If there is some unknown exception I think we should pass it upwards with self.set_exception().

src/artemis/devices/system_tests/test_gridscan_system.py

callumforrester · 2022-01-07T09:13:56Z

@graeme-winter and @DominicOram

Prefer pre-commit to working it out in a CI system as keeps the history cleaner... and makes for easier to read diffs.

I prefer both because unknown agents can get around pre-commit, but pre-commit covers the 90% case so probably okay. However I suggest making a CI job for the tests regardless.

thomascobb · 2022-01-10T13:53:30Z

I'm going to put a plug in for how we do things in Controls, feel free to adopt whatever is useful:

Pipenv for version management
Pre-commit with black, flake8, isort and mypy for static analysis
Pytest for code and coverage
Sphinx for tutorials, how-to guides, explanations and reference documentation
GitHub Actions for code and docs CI and deployment to PyPI and GitHub Pages
VSCode settings using black, flake8, isort and mypy on save

None of this depends on internal DLS infrastructure so will still work if you unset the PYPI mirror.

We've captured this setup in a skeleton module that you can either copy/paste from or git pull changes from:
https://github.com/dls-controls/dls-python3-skeleton

Here's an example of a module that uses it:
https://github.com/dls-controls/sphinx_rtd_theme_github_versions

…sponsibility

DominicOram · 2022-01-11T12:18:51Z

Done the rework from both @callumforrester and @graeme-winter, thanks! CI issues will be addressed in https://jira.diamond.ac.uk/browse/MXGDA-3761

DominicOram · 2022-01-12T11:33:10Z

After discussing with @graeme-winter today I think this is just waiting on review from @bentom08, if you have any thoughts?

bentom08

Everything looks fine to me, happy for this to be merged in as is.

…rce/MXGDA_3724 MXDGA-3724: Added the MVP for the fast grid scan motion NOTE: Commit originally came from https://github.com/DiamondLightSource/python-artemis

(#2) Move devices in from Artemis

DominicOram added 4 commits December 22, 2021 17:18

MXGDA_3724: Added basic implementation for fast_grid_scan device

5c5a648

MXGDA_3724: Added some unit tests for fast_grid_scan

2f2bc17

MXGDA_3724: Added more unit tests for fast grid scan

cdfbf58

MXGDA_3724: Minor tidy up

4e612ce

graeme-winter reviewed Jan 6, 2022

View reviewed changes

MXGDA_3724: Tidy up after review and add support for checking against…

2c73025

… motor limits

DominicOram requested review from graeme-winter and bentom08 January 6, 2022 17:36

callumforrester suggested changes Jan 7, 2022

View reviewed changes

MXGDA 3724: Fixed fraction direction and moved wait_for_connection re…

9b0cb1e

…sponsibility

DominicOram requested a review from callumforrester January 11, 2022 12:17

MXGDA 3724: Fixed issue with initial setting on fast grid scan status

b33c656

callumforrester approved these changes Jan 11, 2022

View reviewed changes

bentom08 approved these changes Jan 13, 2022

View reviewed changes

DominicOram merged commit 586f62d into main Jan 14, 2022

DominicOram deleted the MXGDA_3724 branch January 14, 2022 11:00

rtuck99 pushed a commit that referenced this pull request Dec 14, 2023

Merge pull request #17 from DiamondLightSource/add-code-from-artemis

9177695

(#2) Move devices in from Artemis

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MXDGA-3724: Added the MVP for the fast grid scan motion #2

MXDGA-3724: Added the MVP for the fast grid scan motion #2

DominicOram commented Jan 4, 2022

graeme-winter left a comment

graeme-winter Jan 6, 2022

DominicOram Jan 6, 2022

graeme-winter Jan 6, 2022

DominicOram Jan 6, 2022

DominicOram Jan 6, 2022

callumforrester Jan 7, 2022

DominicOram commented Jan 6, 2022

graeme-winter commented Jan 7, 2022

callumforrester left a comment

callumforrester Jan 7, 2022

callumforrester commented Jan 7, 2022

thomascobb commented Jan 10, 2022

DominicOram commented Jan 11, 2022 •

edited

Loading

DominicOram commented Jan 12, 2022

bentom08 left a comment

MXDGA-3724: Added the MVP for the fast grid scan motion #2

MXDGA-3724: Added the MVP for the fast grid scan motion #2

Conversation

DominicOram commented Jan 4, 2022

graeme-winter left a comment

Choose a reason for hiding this comment

graeme-winter Jan 6, 2022

Choose a reason for hiding this comment

DominicOram Jan 6, 2022

Choose a reason for hiding this comment

graeme-winter Jan 6, 2022

Choose a reason for hiding this comment

DominicOram Jan 6, 2022

Choose a reason for hiding this comment

DominicOram Jan 6, 2022

Choose a reason for hiding this comment

callumforrester Jan 7, 2022

Choose a reason for hiding this comment

DominicOram commented Jan 6, 2022

graeme-winter commented Jan 7, 2022

callumforrester left a comment

Choose a reason for hiding this comment

callumforrester Jan 7, 2022

Choose a reason for hiding this comment

callumforrester commented Jan 7, 2022

thomascobb commented Jan 10, 2022

DominicOram commented Jan 11, 2022 • edited Loading

DominicOram commented Jan 12, 2022

bentom08 left a comment

Choose a reason for hiding this comment

DominicOram commented Jan 11, 2022 •

edited

Loading