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

HW: Folder restructuring #138

Merged

Conversation

AhmetCanSolak
Copy link
Contributor

This PR restructures the folder in hardware subpackage. The proposed structure is not necessarily final, can be subject to change in the future. Also, this PR creates the first abstract class for camera subpackage (whose content can be populated in another PR). @keithchev and @ieivanov please let me know what do you think on the subfolder structure I introduced here.

Copy link
Contributor

@keithchev keithchev left a comment

Choose a reason for hiding this comment

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

I think this kind of organization makes a lot of sense. One question that it raises is whether to include the subpackage names (cameras, daqs, filters, etc) in the module names (e.g. orca_camera vs orca, ni_daq vs ni, etc). I don't have strong opinion but would generally favor not including the subpackage names, since I think the directory structure and import statements are sufficiently unambiguous. The one thing I feel strongly about is to pick a convention and enforce it (which would potentially mean, in this PR, renaming orca_camera to orca and asi_stage to asi).

A related issue is the specificity of the device subpackage names. It may be wise to include some model number or version in the interest of avoiding future namespace collisions. For example, this could mean using something more specific than asi for the ASI stage. But I can imagine the trade-off between specificity and verbosity is device-dependent, so it's hard to define conventions for this.

@AhmetCanSolak
Copy link
Contributor Author

I think this kind of organization makes a lot of sense. One question that it raises is whether to include the subpackage names (cameras, daqs, filters, etc) in the module names (e.g. orca_camera vs orca, ni_daq vs ni, etc). I don't have strong opinion but would generally favor not including the subpackage names, since I think the directory structure and import statements are sufficiently unambiguous. The one thing I feel strongly about is to pick a convention and enforce it (which would potentially mean, in this PR, renaming orca_camera to orca and asi_stage to asi).

I totally agree with you. I will make updates accordingly(like renaming orca_camera to orca). I am wondering how @ieivanov feels about this convention we are choosing.

A related issue is the specificity of the device subpackage names. It may be wise to include some model number or version in the interest of avoiding future namespace collisions. For example, this could mean using something more specific than asi for the ASI stage. But I can imagine the trade-off between specificity and verbosity is device-dependent, so it's hard to define conventions for this.

Specificity won't be easy to clarify with subpackage names as you pointed out. I am more inclined to being less verbose and more generic(given what I saw from vendor drivers). Possibly this issue deserves a bigger independent discussion. I am also open to evaluate subpackages case-by-case.

@ieivanov
Copy link
Collaborator

ieivanov commented Jan 4, 2023

I totally agree with you. I will make updates accordingly(like renaming orca_camera to orca). I am wondering how @ieivanov feels about this convention we are choosing.

I do like this naming convention.

I would further add that the subpackage names should match the names of the abstract classes we implement, as we discussed in #136. This may mean renaming daqs to io_devices (DAQ stands for Data Acquisition, which is not exactly how we're using these cards) and filters to state_devices, depending on the convention we pick and how we implement the abstract classes. In this case the types of abstract classes we support would dictate the folder structure.

I'm not sure how to deal with things like pumps - maybe we should have a other category, which would not have to inherit an abstract class? I think this becomes a question of how many subpackages copylot would support and if we can seamlessly extend them.

@AhmetCanSolak
Copy link
Contributor Author

I would further add that the subpackage names should match the names of the abstract classes we implement, as we discussed in #136. This may mean renaming daqs to io_devices (DAQ stands for Data Acquisition, which is not exactly how we're using these cards) and filters to state_devices, depending on the convention we pick and how we implement the abstract classes. In this case the types of abstract classes we support would dictate the folder structure.

I am open to rename subpackages @ieivanov. Also, the names chosen in this PR doesn't need to be final. As we have implementations and abstract classes, we can refactor subpackage names as needed.
For the daqs, I went with it, since many vendors call them DAQ devices (despite these devices usually also have analog and digital voltage output capabilities). I would suggest voltage_io maybe? So it won't be confused with file IO or other sorts of IO operations.
For the state_devices, what are the other types of devices do you think fall into the same category with filters @ieivanov ?

I'm not sure how to deal with things like pumps - maybe we should have a other category, which would not have to inherit an abstract class? I think this becomes a question of how many subpackages copylot would support and if we can seamlessly extend them.

I would like to avoid other or misc device subpackage for as long as possible(I know it is not easy). With more software development around microfluidic devices and so, I feel pumps might have more implementations and their own abstract class eventually, or potentially we can merge with motor controller adapters like the ones in PyMotors.

@ieivanov
Copy link
Collaborator

ieivanov commented Jan 5, 2023

I would suggest voltage_io maybe? So it won't be confused with file IO or other sorts of IO operations.

I didn't think of that. Let's stick with daqs then.

For the state_devices, what are the other types of devices do you think fall into the same category with filters

It's true that filters would cover most of the devices in this category. Another example of a state device would be a flip mirror, which is not exactly a filter. But maybe it's better to stick with a more descriptive name like filters rather than a more general name like state_devices.

With more software development around microfluidic devices and so, I feel pumps might have more implementations and their own abstract class eventually, or potentially we can merge with motor controller adapters like the ones in PyMotors.

Good point!

@AhmetCanSolak
Copy link
Contributor Author

I would suggest voltage_io maybe? So it won't be confused with file IO or other sorts of IO operations.

I didn't think of that. Let's stick with daqs then.

I will merge this PR with daqs then but I later we can still rename it. voltage_io, signal_io or something else? we can keep this in the back of our minds.

For the state_devices, what are the other types of devices do you think fall into the same category with filters

It's true that filters would cover most of the devices in this category. Another example of a state device would be a flip mirror, which is not exactly a filter. But maybe it's better to stick with a more descriptive name like filters rather than a more general name like state_devices.

I didn't think of that. Do you have a flip mirror that we can write an adapter for? If so let's make an issue on it and add it. After we see device adapter implementations for filterwheels and flip mirrors it might be easier to merge everything together into a single subpackage.

@AhmetCanSolak AhmetCanSolak merged commit 3f89139 into czbiohub-sf:main Jan 5, 2023
@AhmetCanSolak AhmetCanSolak deleted the hw-folder-restructuring branch January 5, 2023 18:44
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.

3 participants