-
Notifications
You must be signed in to change notification settings - Fork 26
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
Split out common AD file plugin logic into core writer class, create ADTiffWriter #606
Open
jwlodek
wants to merge
31
commits into
bluesky:main
Choose a base branch
from
jwlodek:ad-tiff-writer
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
652de13
Starting to work on ad tiff writer
jwlodek e289ee4
Resolve merge conflicts
jwlodek f36ec3a
Continue working on tiff writer
jwlodek 83dff62
Further work on tiff writer, existing tests now passing.
jwlodek 1a52a21
Remove functions moved to superclas from hdf writer
jwlodek 489cfd8
Significant re-org and simplification of ad classes
jwlodek 83c6884
Ruff formatting
jwlodek 3b4f45a
Modify ad sim classes to reflect new superclasses
jwlodek 7175b30
Modify vimba and kinetix classes
jwlodek faf53d6
Modify aravis and pilatus classes
jwlodek 5b9f60f
Update all tests to make sure they still pass with changes
jwlodek 8bbfd0e
Some cleanup
jwlodek 1eab818
Merge with upstream
jwlodek f6825b4
Changes to standard detector to account for controller/writer types i…
jwlodek 651b80d
Significant changes to base detector, controller, and writer classes
jwlodek 38a61e8
Update detector and controller classes to reflect changes
jwlodek aecdf04
Make sure panda standard det uses new type hints
jwlodek e42fa12
Most tests passing
jwlodek 07684a4
Merge with main and resolve conflicts
jwlodek 6dc09f3
Revert change in test that was resolved by pydantic version update
jwlodek 1f7dcd7
Remove debugging prints
jwlodek 35dd1b1
Linter fixes
jwlodek 8112220
Fix linter error
jwlodek ac1e509
Move creation of writer outside of base AreaDetector class init per r…
jwlodek 8494da4
Make sure we don't wait for capture to be done!
jwlodek b212432
Merge with upstream
jwlodek 3242d45
Merge with upstream
jwlodek 488d7eb
Allow for specifying whether or not to use fileio signals for dataset…
jwlodek a76b70f
Revert "Allow for specifying whether or not to use fileio signals for…
jwlodek 7da935e
Fix linter errors, remove unused enum
jwlodek 0cd0ddf
Change from return to await to conform to return type
jwlodek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
from collections.abc import Sequence | ||
from typing import cast | ||
|
||
from bluesky.protocols import HasHints, Hints | ||
|
||
from ophyd_async.core import PathProvider, SignalR, StandardDetector | ||
|
||
from ._core_io import ADBaseIO, NDFileHDFIO, NDFileIO | ||
from ._core_logic import ADBaseController, ADBaseDatasetDescriber | ||
from ._core_writer import ADWriter | ||
from ._hdf_writer import ADHDFWriter | ||
from ._tiff_writer import ADTIFFWriter | ||
|
||
|
||
def get_io_class_for_writer(writer_class: type[ADWriter]): | ||
writer_to_io_map = { | ||
ADWriter: NDFileIO, | ||
ADHDFWriter: NDFileHDFIO, | ||
ADTIFFWriter: NDFileIO, | ||
} | ||
return writer_to_io_map[writer_class] | ||
|
||
|
||
class AreaDetector(StandardDetector, HasHints): | ||
_controller: ADBaseController | ||
_writer: ADWriter | ||
|
||
def __init__( | ||
self, | ||
prefix: str, | ||
path_provider: PathProvider, | ||
writer_class: type[ADWriter] = ADWriter, | ||
writer_suffix: str = "", | ||
controller_class: type[ADBaseController] = ADBaseController, | ||
drv_class: type[ADBaseIO] = ADBaseIO, | ||
drv_suffix: str = "cam1:", | ||
name: str = "", | ||
config_sigs: Sequence[SignalR] = (), | ||
**kwargs, | ||
): | ||
self.drv = drv_class(prefix + drv_suffix) | ||
self._fileio = get_io_class_for_writer(writer_class)(prefix + writer_suffix) | ||
|
||
super().__init__( | ||
controller_class(self.drv, **kwargs), | ||
writer_class( | ||
self._fileio, | ||
path_provider, | ||
lambda: self.name, | ||
ADBaseDatasetDescriber(self.drv), | ||
), | ||
config_sigs=(self.drv.acquire_period, self.drv.acquire_time, *config_sigs), | ||
name=name, | ||
) | ||
|
||
@property | ||
def controller(self) -> ADBaseController: | ||
return cast(ADBaseController, self._controller) | ||
|
||
@property | ||
def writer(self) -> ADWriter: | ||
return cast(ADWriter, self._writer) | ||
|
||
@property | ||
def hints(self) -> Hints: | ||
return self._writer.hints |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of inserting another class into the heirarchy with many more arguments, could we move out the creation of the driver and writer classes into a utility function instead? I had a go with this idea and came up with:
I did some of the changes required so pyright was happy, but didn't make the tests work:
efd86c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jwlodek#2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to have the utility function(s) be factories that take in a drv_cls and writer_cls? It would be nice if we didn't need to make a function for each writer type. I am OK with that approach generally though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable, I wonder if we should stretch it even further and say that we make one
AravisDetector
that has an enum init argfileio_type
that switches between HDF and TIFF?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be ideal I think. I'll work on getting something like that working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, not sure I love that, but it would be nice to be able to specify the writer and have a default suffix. What if
fileio_suffix=None
, andAnd have the enum values for the writer types just correspond to the default suffixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise we use whatever the suffix kwarg provides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found another possible approach which avoids the need to make detector specific subclasses:
So now
det.drv
anddet.fileio
are always present, and we can dodet.get_plugin("stats", NDPluginStatsIO)
if we created it withAreaDetector(..., plugins={"stats": NDPluginStatsIO(prefix+"STATS")}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the detector specific classes turn into helper functions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed again to jwlodek#2