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

Updating service docs (Meiji) #197

Merged
merged 13 commits into from
May 29, 2024
Merged

Conversation

meiji274
Copy link
Contributor

@meiji274 meiji274 commented May 8, 2024

This is part of the team effort to fill out the documentation pages in the services module of catkit2 (see issue #190 for more information).

So far I am planning to do 3 services (will update if more are needed):
-[x] ZWO camera
-[x] Newport picomotor
-[x] Web power switch

@meiji274 meiji274 requested a review from ivalaginja May 8, 2024 21:01
@meiji274 meiji274 self-assigned this May 8, 2024
@meiji274 meiji274 changed the title Updating docs pages (Meiji) Updating service docs (Meiji) May 8, 2024
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up, just leaving you some comments before you move on.

Comment on lines 4 to 8
This service operates a ZWO camera. There are currently four different types of ZWO cameras being used on the testbed:
- ZWO ASI533MM (Science camera, https://www.zwoastro.com/product/asi533mm-mc/)
- ZWO ASI290MM (TA camera, https://agenaastro.com/zwo-asi290mm-cmos-monochrome-astronomy-imaging-camera.html)
- ZWO ASI178MM (MOWFS Camera and Pupil Camera, https://agenaastro.com/zwo-asi178mm-cmos-monochrome-astronomy-imaging-camera.html)
- ZWO ASI1600MM (Phase Retrieval Camera, https://agenaastro.com/zwo-asi1600mm-p-cmos-monochrome-astronomy-imaging-camera-pro.html)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation should not refer to a specific testbed. Instead, there should just be a list of concrete devices (the ones you list here) that have been tested and used with catkit2 so far.

science_camera:
service_type: zwo_camera
simulated_service_type: camera_sim
interface: hicat_camera
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too, replace the hicat reference with either a default proxy that lives inside of catkit2, or if there is none, just delete this line (a dedicated proxy is not required for a service).

Comment on lines 31 to 33
exposure_time_step_size: 33
exposure_time_offset_correction: -79.2
exposure_time_base_step: 50
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines can go as well as they relate to the hicat camera proxy only.

exposure_time_offset_correction: -79.2
exposure_time_base_step: 50

ta_camera:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We only need one entry as an example. Remember, these are the catkit2 docs, not the hicat docs.

Comment on lines 59 to 66
``take_calibrated_exposures(num_exposures, only_use_cache=True)``: This takes a number of calibrated exposures.

``get_exposure_calibration_function(only_use_cache=True)``: This gets a function to calibrate individual exposures.

``take_calibrated_image(num_exposures, upsample_factor=None, alignment_window=None, only_use_cache=True)``: This takes a calibrated
image which is background subtracted, exposure time corrected, nd flux calibrated and sub-frame aligned using phase cross correlation method.

``take_dark(num_exposures)``: This takes a dark image for the current exposure time by moving the beam dump.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Careful, this is again from the HiCAT proxy, so these need to be deleted. If you want to make a note about the generic camera proxy, then it might be best to do this in the general description up top in this doc.

@meiji274 meiji274 mentioned this pull request May 14, 2024
21 tasks
@meiji274 meiji274 marked this pull request as ready for review May 22, 2024 21:15
@meiji274 meiji274 force-pushed the feature/add_service_docs_meiji branch from c98f813 to 6d93913 Compare May 22, 2024 21:16
@ivalaginja ivalaginja marked this pull request as draft May 23, 2024 14:23
@ivalaginja ivalaginja force-pushed the feature/add_service_docs_meiji branch from 6d93913 to 503f353 Compare May 23, 2024 14:29
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Looking good except for one minor comment.

Comment on lines 6 to 8
For some examples of commercially available web power switches:
https://controlbyweb.com/webswitch/
https://dlidirect.com/products/new-pro-switch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you list the ones that have been run successfully with catkit2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@meiji274 meiji274 marked this pull request as ready for review May 23, 2024 17:20
@@ -1,14 +1,48 @@
Web Power Switch
================

This service controls a power switch that is controllable over the internet.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's for a very specific web power switch. Look up which one we use.

Copy link
Contributor Author

@meiji274 meiji274 May 23, 2024

Choose a reason for hiding this comment

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

I asked Raphael and it should be the link below that he sent me (line 7).


Datastreams
-----------
``{outlet_name}``: The name of an outlet added by the user using the add_outlet() command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong. The outlet names are defined by the config.

Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Only minor inline comments, but I also just noticed that your links are not formatted URLs - for rst files, you need to declare/format them as described in this link, otherwise they are unclickable (see also examples in other service docs):
https://sublime-and-sphinx-guide.readthedocs.io/en/latest/references.html#links-to-external-web-pages

Comment on lines 43 to 45
``{axis_name}_command``: A movement command along the axis corresponding to axis_name (where axis_name can be x, y, or z).

``{axis_name}_current_position``: The current position of the picomotor along the axis corresponding to axis_name (where axis_name can be x, y, or z).
Copy link
Collaborator

Choose a reason for hiding this comment

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

In line with Emiel's comment further below, let's also specify here that the axis names are defined in the service configuration.

Comment on lines 6 to 7
So far catkit2 has been tested on the LPC7-PRO web power switch from TeleDynamics:
https://www.teledynamics.com/#/productdetails/LPC7-PRO
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is minor, but could you reformat this into a bullet list like you did for the ZWO camera models? This is also how we have it in some other service docs and makes it simpler to extend later on (and to find where to extend).

@@ -1,14 +1,64 @@
ZWO Camera
==========

This service operates a ZWO camera. There are currently four different types of ZWO cameras that have been tested and used with catkit2 so far:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditch the "four" to limit the risk of incomplete list extension in the future, put down something like "the following devices" instead.

@@ -1,14 +1,64 @@
ZWO Camera
Copy link
Collaborator

Choose a reason for hiding this comment

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

It needs to be stated here that using the ZWO cameras requires the installation of their drivers - you can copy that information from here, and delete the comment from the env file:

- zwoasi>=0.0.21 # Requires additional manual install of driver(s) from https://astronomy-imaging-camera.com/software-drivers


https://www.newport.com/f/open-loop-picomotor-motion-controller

The bottom of the webpage linked above has resources for the user including software downloads, technical notes, and user manuals.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please state more clearly whether for example a driver needs to be installed for this device or not.

@@ -1,14 +1,47 @@
Web Power Switch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, can you confirm no driver installation is required, or alternately state here that it is required?

@meiji274 meiji274 force-pushed the feature/add_service_docs_meiji branch from ac247c9 to e8538cd Compare May 24, 2024 17:51
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Almost there - there's one language fix I'd like to be addressed. I also left two comments asking you to go slightly out of scope of this PR but it's minor and is still related, it would be great if you could implement those too.


``brightness``: Brightness of the camera.

``width``: The width of the camera.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that the description of width and height, as well as x and y offset are not really clear - it was me who wrote this for the Alvium and Hamamatsu camera but let's use this opportunity to fix this.

Width and height should be "of the camera frames." or "images" if you like that better.
Offset x and y should be: "... of the camera frames on the sensor." or anything that specifies better.

Could I also ask you to sneak in a fix for this in the Alvium and Hamamatsu service doc please?


Datastreams
-----------
``temperature``: The temperature as measured by this sensor in Celsius.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's replace "this sensor" by "the camera", and also port that change to the Alvium and Hamamatsu service doc.

- `ZWO ASI1600MM <https://agenaastro.com/zwo-asi1600mm-p-cmos-monochrome-astronomy-imaging-camera-pro.html>`_

For camera specs, see the website links above.
Note that to use the ZWO cameras requires additional manual install of driver(s) from `zwoastro.com <https://astronomy-imaging-camera.com/software-drivers>`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a wonky sentence, please fix to: "Note that using ZWO cameras requires a manual installation of..."

Properties
----------
``exposure_time``: Exposure time of the camera.
Copy link
Collaborator

Choose a reason for hiding this comment

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

One last one: please add the exposure time units here.

@ivalaginja ivalaginja merged commit dc0c024 into develop May 29, 2024
6 checks passed
@ivalaginja ivalaginja deleted the feature/add_service_docs_meiji branch May 29, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants