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

Introduce PvaAbstractions and use them in SeqTable #522

Conversation

evalott100
Copy link
Contributor

Closes #442 and #310

@evalott100 evalott100 self-assigned this Aug 15, 2024
@evalott100 evalott100 marked this pull request as draft August 15, 2024 10:39
@evalott100 evalott100 linked an issue Aug 15, 2024 that may be closed by this pull request
@evalott100 evalott100 changed the title 442 switch from typeddict to row wise numpy structured data for tables Introduce PvaAbstractions and use them in SeqTable Aug 15, 2024
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from 8bbdcdc to 53d29d4 Compare August 15, 2024 14:26
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from bffc983 to d976d02 Compare August 21, 2024 12:56
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from d976d02 to e3b149c Compare August 21, 2024 13:14
@evalott100 evalott100 marked this pull request as ready for review August 21, 2024 13:14
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from e3b149c to 42555ed Compare August 21, 2024 13:59
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from 42555ed to e68976b Compare August 21, 2024 14:19
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from e68976b to c4bece5 Compare August 21, 2024 14:21
Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

As discussed, please could we have:

SignalType = TypeVar("SignalType", bool, int, ..., BaseModel)

class Signal:
    def __init__(self, datatype: Optional[SignalType] = None):
        check_one_of(datatype, SignalType)
        ...

I forgot that we wanted to take datatype in Signal.__init__ so that we could create a Signal with a type but no backend in create_children_from_annotations, then fill it in on connect.

As per #310 (comment)

tests/core/test_device_save_loader.py Outdated Show resolved Hide resolved
src/ophyd_async/fastcs/panda/_table.py Outdated Show resolved Hide resolved
tests/fastcs/panda/test_panda_connect.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_signal_backend.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_signal_backend.py Outdated Show resolved Hide resolved
Still need to add datatype to the args of signal
@evalott100
Copy link
Contributor Author

evalott100 commented Sep 6, 2024

@coretl

Can we implement #310 (comment) in another issue/PR? This is already a very big change, and changing init/connect args for Signal/SignalBackend touches so many places in the code-base.

Additionally, I'd like to disallow arg -> kwarg inference on Signal and SignalBackend init and connect which we could do in the same PR.

@coretl
Copy link
Collaborator

coretl commented Sep 6, 2024

@coretl

Can we implement #310 (comment) in another issue/PR? This is already a very big change, and changing init/connect args for Signal/SignalBackend touches so many places in the code-base.

Additionally, I'd like to disallow arg -> kwarg inference on Signal and SignalBackend init and connect which we could do in the same PR.

Sounds like a good idea, let me know when this needs another review

@evalott100
Copy link
Contributor Author

New issue here. I'm ready for a new review #562

src/ophyd_async/core/_signal_backend.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_signal_backend.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_soft_signal_backend.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_soft_signal_backend.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_soft_signal_backend.py Outdated Show resolved Hide resolved
src/ophyd_async/fastcs/panda/_table.py Outdated Show resolved Hide resolved
src/ophyd_async/fastcs/panda/_table.py Outdated Show resolved Hide resolved
src/ophyd_async/fastcs/panda/_table.py Outdated Show resolved Hide resolved
src/ophyd_async/fastcs/panda/_table.py Outdated Show resolved Hide resolved
# Wait for pre-delay then open shutter
SeqTableRow(
SeqTable.row(
time1=in_micros(pre_delay),
Copy link
Collaborator

Choose a reason for hiding this comment

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

... as we have an implicitly defined repeats field that will have changes defaults from 1 to 0. Are we lacking a test for this plan stub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have tests for this but at no point in them do we check on repeats

@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from 1aafbc0 to 49a047b Compare September 11, 2024 12:11
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from 02c89b9 to 695e1db Compare September 11, 2024 12:46
@evalott100 evalott100 force-pushed the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch from c2ac27c to b728640 Compare September 11, 2024 14:28
@evalott100 evalott100 merged commit 0cbfbe6 into main Sep 12, 2024
18 checks passed
@evalott100 evalott100 deleted the 442-switch-from-typeddict-to-row-wise-numpy-structured-data-for-tables branch September 12, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants