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

Move FastCS Eiger to fastcs, move odin io/writer to seperate modules outside of eiger #557

Open
jwlodek opened this issue Sep 4, 2024 · 5 comments

Comments

@jwlodek
Copy link
Member

jwlodek commented Sep 4, 2024

Looking at the recent fastcs eiger support, I noticed two things:

epics/eiger/_odin_io.py - This appears to be general to all Odin detectors, is that correct? If yes, I think it should be decoupled from eiger, maybe in fastcs/odin/_odin_io.py and fastcs/odin/_odin_hdf_writer.py or something similar.

Also, should the eiger support be moved under fastcs since it is a fastcs implementation? Then a separate ADEiger version could be placed under epics.

Thoughts?

@DominicOram
Copy link
Contributor

Despite doing the work I have no strong opinions on it's organisation. I defer to @coretl and @GDYendell who know more about the bigger picture than I do, I would just like to get a working ophyd-async eiger...

@coretl
Copy link
Collaborator

coretl commented Sep 5, 2024

In the epics module we take the support module name and PEP8 it, so we have epics/adaravis. Could we do the same with the FastCS modules? Would that be fastcs/odin and fastcs/eiger, or are the repos named something different? @GDYendell what do you think?

@GDYendell
Copy link
Contributor

I think this sounds reasonable.

The repos are currently eiger-fastcs and odin-fastcs. It seems the convention on PyPI package naming is actually to have the core module name at the start, e.g. pytest-*, so maybe we should change that?

@coretl
Copy link
Collaborator

coretl commented Sep 5, 2024

I think this sounds reasonable.

The repos are currently eiger-fastcs and odin-fastcs. It seems the convention on PyPI package naming is actually to have the core module name at the start, e.g. pytest-*, so maybe we should change that?

That probably makes sense...

Then we have fastcs/odin that comes from fastcs-odin. Looks neat

@GDYendell
Copy link
Contributor

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

No branches or pull requests

4 participants