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

Feat: general wrapper #26

Closed
wants to merge 5 commits into from
Closed

Conversation

younesStrittmatter
Copy link
Contributor

Description

a general wrapper that accepts arguments to field mappings as arguments

Type of change

  • feat: A new feature

… field mapping and a string for return value to state field mapping
Copy link
Contributor

@benwandrew benwandrew left a comment

Choose a reason for hiding this comment

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

this is super cool! i hope John Ousterhout* would like the direction we're moving in ;)

one general question for my own understanding (which may be better for me to address in office hours): i'm confused about the relationship between the decorator/wrapper wrap_to_use_state in delta.py and this new wrap_to_use_state_general in wrapper.py. is the latter not ultimately replacing the former?

*reference to author of a book @hollandjg shared a while ago: https://milkov.tech/assets/psd.pdf

@hollandjg
Copy link
Member

I like the look of this!

Would it be sensible to split up the wrapper into two – one which does the input name mapping, and one which does the output wrapping? Then we could use the two independently – perhaps your inner function needs to return two values (say conditions and experiment_js_code) and returns those as a Result/Delta object, but you still want a mapping. Conversely, perhaps you have a function which uses entirely standard naming for the variables, but you want to wrap the outputs.

Copy link
Contributor

@musslick musslick left a comment

Choose a reason for hiding this comment

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

Looks great; just got a minor comment for a name change.

pass


def wrap_to_use_state_general(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the name be shorted, e.g., wrap_to_state? I imagine people will be using this a lot, so we might want to give it a short name.

@hollandjg
Copy link
Member

hey @younesStrittmatter , do you think you could refactor this so that there's a wrapper function which just does the input field-name mapping? We could include that as an option in the on_state function from #33, but it needs to be an independent wrapper function first so that we can test it really extensively on its own.

Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

I like the general idea of this a lot, and have included a lot of the ideas in #33. Would it be possible to make a simple wrapper function which just does the input-name mapping?

@musslick musslick mentioned this pull request Nov 1, 2023
5 tasks
hollandjg added a commit that referenced this pull request Nov 29, 2023
fix: types on state objects for controller
@hollandjg
Copy link
Member

I think this is covered by the newest version of the state object – closing this now.

@hollandjg hollandjg closed this Dec 1, 2023
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.

4 participants