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

Process keywords #177

Closed
wants to merge 6 commits into from
Closed

Process keywords #177

wants to merge 6 commits into from

Conversation

eagmon
Copy link
Member

@eagmon eagmon commented Apr 7, 2022

This draft PR includes work towards improved handling of Process parameters as keywords (#171)


By creating this pull request, I agree to the Contributor License
Agreement, which is available in CLA.md at the top level of this
repository.

Comment on lines 36 to 40
**base_parameters):

parameters = locals().copy()
base_parameters = parameters.pop('base_parameters')
self.initialize(parameters, base_parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I helped create this thing, but on reflection I feel the cost here for Process creators to have to add this four-line litany to every __init__ method is too high. I appreciate what we're trying to solve here but the trade-off can't be to create cryptic things we demand of our users.... IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can make this optional? If people want function keywords, they can follow this template. But the previous __init__ approach with defaults and a parameters dict can work just as well.

"""
_ = states
return self.parameters['time_step']
return self.parameters['timestep']
Copy link
Contributor

Choose a reason for hiding this comment

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

timestep change is good, maybe a separate PR?

@eagmon eagmon closed this Apr 22, 2022
@eagmon eagmon deleted the process-locals branch August 5, 2022 19:19
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