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

add signal builders interface #29

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

1024bees
Copy link
Contributor

this is a first pass PR at adding "derived" signals in a fairly well defined way, in both python and rust

in python, we have a working test for the "slice" case -- while we could support this with custom code in rust, i think its a decent exercise for showing how we can use python logic to create a new signal

in rust, we have a bunch of unused code -- the SimpleLazy example is unused in tests, and the ClockedSignal has an ignored test that isnt fully implemented.

overall, i think the python code is in a much better state than the rust code (the test isn't fully implemented), and if we want to merge it quickly then we should probably remove the derived_signals rust code for now

look at the tests for examples of these apis

Copy link
Owner

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

This looks great! However, I would like to simplify the review. Since you yourself already indicated that the Rust stuff isn't quite done, how about you isolate all the changes that are necessary for the functionality demonstrated in the Python test_slice() function and make a PR with that. Then we can discuss and work on the Rust stuff as a separate issue.

@ekiwi
Copy link
Owner

ekiwi commented Jan 7, 2025

@1024bees feel free to ping me if you need help with this! Thanks for working on the python bindings.

@1024bees
Copy link
Contributor Author

1024bees commented Jan 8, 2025

doing a little bit of wandering on it :) pipecleaning it with https://github.com/1024bees/dang, will shoot to have it wrapped up this weekend knowing that youre Looking.

@ekiwi
Copy link
Owner

ekiwi commented Jan 8, 2025

doing a little bit of wandering on it :) pipecleaning it with https://github.com/1024bees/dang, will shoot to have it wrapped up this weekend knowing that youre Looking.

Perfect! Please don't feel any pressure vis-à-vis timeline. The one thing I am asking for is that you try to keep your PRs relatively small.

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.

2 participants