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.

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

Update I24 serial docs #487

Merged
merged 21 commits into from
Sep 25, 2024
Merged

Update I24 serial docs #487

merged 21 commits into from
Sep 25, 2024

Conversation

noemifrisina
Copy link
Contributor

No description provided.

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Thanks, some comments

Comment on lines +158 to +162
**NOTE** As of version ``1.0.0``, the ``Set parameters`` button has been removed and
the parameters will now be read from the edm and applied to the collection directly
once the ``Start`` button is pressed. For previous versions however, the button must
still be pressed before starting the collection. A copy of the parameter file and chip
map (if applicable) will still be saved in the data directory at collection time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: I think docs should usually just reflect the state of things as they currently are rather than as they were. Unless you're worried that the person reading this may be used to the older way of running it?

Copy link
Contributor Author

@noemifrisina noemifrisina Sep 13, 2024

Choose a reason for hiding this comment

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

In general, I agree with just describing the latest version. In this case, I am a bit worried as scientists and users have been used until now to having to press the set parameters button for the collection to work. I think it may be worth leaving the comment for a bit while the staff gets used to it and then remove it at a later time - hopefully by the beginning of next run...

- Close to being completed (see issue #44)

2. Refactor and fix the logger.
3. Use callbacks for file IO eg. to write parameter and map files.
Copy link
Contributor

Choose a reason for hiding this comment

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

And for:

  • ispyb deposition
  • nexus writing

Comment on lines 57 to 58
* - Use Eiger device and bluesky plans for detector setup
- Jan. 25
Copy link
Contributor

Choose a reason for hiding this comment

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

So is this assuming that the FastCS Eiger work is already done and not being done as part of this project? If so we should link the issues and be explicit that it's not under the scope of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, here I was referring only to converting the current set up code into bluesky plans but admittedly I wasn't very clear. I'll rephrase and specify that it's dependent on those issues being completed first

Comment on lines 60 to 61
* - Start moving from edm to web GUI
-
Copy link
Contributor

Choose a reason for hiding this comment

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

"Start moving" is a bit vague. I would break it into specific things like "move the OAV viewer" and give them dates

- Reinstate removed code from sacla and move it to bluesky.
- Add any plans/devices that might be needed for other locations.

8. Reinstate full mapping code using bluesky.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this all assumes that we do not change how we run the SSX. Are there any plans for integrating panda and coming up with a more standard PVT way of running the scans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Integrating Panda, yes, at least for the fixed target, but I have no idea yet of a possible timeline so I was going to add it at a later time.

Copy link
Contributor

Choose a reason for hiding this comment

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

A comment to that affect would be good

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

One comment in "code" and I also think it would be good to mention the panda somewhere here. Otherwise good, thanks.

Comment on lines 75 to 77
* - Set up `coniql <https://github.com/DiamondLightSource/coniql>`__ on the beamline
- Jan. 25
- :material-regular:`pending;2em`
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe coniql is now not what we want to use, I'm discussing with people at https://diamondlightsource.slack.com/archives/C06BBH6LX6U/p1727179118445009

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@3c0a8c9). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #487   +/-   ##
=======================================
  Coverage        ?   77.90%           
=======================================
  Files           ?       89           
  Lines           ?     6711           
  Branches        ?        0           
=======================================
  Hits            ?     5228           
  Misses          ?     1483           
  Partials        ?        0           
Components Coverage Δ
i24 SSX 57.12% <ø> (?)
hyperion 96.25% <ø> (?)
other 94.79% <ø> (?)

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@noemifrisina noemifrisina merged commit 25ce1fd into main Sep 25, 2024
21 checks passed
@noemifrisina noemifrisina deleted the update-docs branch September 25, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants