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

API discussion #233

Closed
jlstevens opened this issue Jun 14, 2018 · 12 comments
Closed

API discussion #233

jlstevens opened this issue Jun 14, 2018 · 12 comments
Labels
API Improvement to API or something like that component: "parameterized" help me name this :) status: discussion Discussion. Not yet a specific feature/bug. Likely to result in multiple PRs/issues.
Milestone

Comments

@jlstevens
Copy link
Contributor

jlstevens commented Jun 14, 2018

PR #230 gives us an opportunity to clean up the namespace of parameterized objects but it also gives us a chance to tidy up the rest of the API: we can define the API we want for the future on .param while retaining backwards compatibility.

This issue is intended to start some discussion. Here are some of my own notes:

Notes

  1. Some API is currently staying on parameterized objects e.g pprint, script_repr, instance (for ParameterizedFunctions) and .param itself of course. There is also a .initialized attribute that I think should be private. What should be exposed at this level?
  2. Do we want to keep script_repr in future at the parameterized level?
  3. The way .params() sometimes returns a dictionary and sometimes does not has always seemed odd to me.
  4. set_default seems odd as you might want to set other parameter attributes too. Shouldn't there be a more general way to set any parameter attribute?
  5. Generally there is a fair bit of redundancy I think could be eliminated.

What are your own thoughts on this issue?

@jbednar
Copy link
Member

jbednar commented Jun 14, 2018

  1. I vote strongly for retaining .param and .instance(), and for making .initialized private unless someone finds some clear reason not to. I don't have any better suggestion than .param, so I vote to keep that as well. I vote weakly to keep .pprint() and .script_repr(), but would be happy to rename them if a better name is available.
  2. I'd be happy to hide script_repr under .param, though the implementation would need to stay on Parameterized (e.g. as ._script_repr).
  3. For me to have an opinion about .params, I'd have to see a proposed alternative (e.g. two methods?).
  4. I don't know where set_default is used or what it's used for, but yes, it does seem like a more general mechanism should be available.
  5. Presumably someone would need to have a task of going through and looking for such redundancy and proposing how to eliminate it.

@jlstevens
Copy link
Contributor Author

jlstevens commented Jun 14, 2018

I vote strongly for retaining .param ...

Do you mean .params(...) or .param (the namespace sub-object, no parentheses)? Or are you suggestion that by making Parameters callable it could offer a useful API that way?

@jbednar
Copy link
Member

jbednar commented Jun 14, 2018

Under 1, I'm talking about the namespace subobject, as I think you were. Under 3. I'm talking about .params(), where we should drill down a bit with two possible replacements for .param.params()'s functionality of returning one parameter by name or a dictionary of all parameters:

3a. Rename .param.params() to just .param() but otherwise leave it unchanged (returning a parameter if one name is specified, and otherwise the full dictionary)
3b. Provide .param.objects() to return a dictionary, and make .param(name) return a single Parameter object by name. (Or maybe .param[name] or even .param.name for that)? Then .param.values() can replace param.get_param_values().

Or...?

@jbednar
Copy link
Member

jbednar commented Jun 14, 2018

  1. Should remove the "backwards compatibility" feature where .set_param accepts positional arguments as a special case.
  2. Rename methods that redundantly include the word "param":
    • 7a .param.set_param() to .param.set()
    • 7b .param.print_param_values() to .param.print_values() (or perhaps delete this method?)
  3. Move functionality that seems to be about individual Parameters onto Parameter instead of Parameterized:
    • 8a .force_new_dynamic_value()
    • 8b .get_value_generator()
    • 8c .inspect_value()
  4. Currently public API that seems like it could be made private:
    • 9a: state_push, state_pop

@jlstevens
Copy link
Contributor Author

One thing we mentioned when discussing #230 is that we might want decorators for methods we decide should not have been made public i.e this might be a tool to help us tighten up the API.

@jbednar
Copy link
Member

jbednar commented Jun 14, 2018

  1. Remove .script_repr() methods, replacing with pprint or by somehow making __repr__ support pretty printing?

@jbednar
Copy link
Member

jbednar commented Jun 14, 2018

One thing we mentioned when discussing #230 is that we might want decorators for methods we decide should not have been made public i.e this might be a tool to help us tighten up the API.

Why decorators? Presumably they just wouldn't be public in the .param object, even if they were previously public when they were in Parameterized?

@ceball
Copy link
Contributor

ceball commented Jun 25, 2018

At some point I will compile a list of proposed changes out of all the various comments on various issues. For now, I'll just pile on some additional things I'd like to consider:

  • _x_param_value etc

@ceball ceball added API Improvement to API or something like that status: discussion Discussion. Not yet a specific feature/bug. Likely to result in multiple PRs/issues. component: "parameterized" help me name this :) labels Apr 13, 2020
@jbednar jbednar mentioned this issue May 18, 2020
@tonyfast tonyfast added this to the 2.0 milestone Aug 24, 2020
@philippjfr
Copy link
Member

philippjfr commented Sep 14, 2020

Main API

Core

.values: Dictionary-like accessor of parameter values (NEW: agreed upon, should match defaults and be made callable?)
.objects: Return parameter objects as a dictionary (should allow making indexable?)

instance_params = {p: parameterized.param[p] for p in parameterized.param}
instance_params = parameterized.param.objects(instance=True) # default

class_params = parameterized.param.objects(instance=False)
class_params =  {p: type(parameterized).param[p] for p in parameterized.param}

existing_params = parameterized.param.objects(instance='existing')

.views: Get immutable parameter views that allow accessing but not modifying parameter attributes (NEW: replace objects?)

.defaults: Dictionary-like accessor of parameter defaults (NEW: currently method, delete instead?!)

.changed: Return an iterator of changed parameter names (NEW: not yet agreed, allow making it an option on the values method instead?)

Other

.log: Log with various log-levels (NEW: agreed upon)
.outputs: Returns names of parameters method declared as output of this object
.update: Updates parameter values (NEW: agreed upon)

Serialization

.deserialize_value
.deserialize_parameters
.schema
.serialize_value
.serialize_parameters

Watch

.params_depended_on
.trigger
.unwatch
.watch

Misc

.script_repr
.pprint

dynamic

.get_value_generator
.inspect_value
.force_new_dynamic_value
.set_dynamic_time_fn
.state_push
.state_pop

Removed

.params
.set_default
.get_param_values
.print_param_defaults
.print_param_values
.warning
.message
.verbose
.debug

@jbednar
Copy link
Member

jbednar commented Sep 14, 2020

The above list is only a draft, with lots of questions. The basic goal was to see which methods didn't need to be there at all ("Removed"), and which ones could be replaced with general-purpose Pythonic support under the idea that obj.param should act like a Python dictionary of Parameter objects as closely as possible. I.e., can we eliminate special-purpose API by instead trying to make the object act more closely like a dictionary, as much as is practical?

Why would it not be practical? Well, under the covers .param it is not simply a dictionary of Parameter objects, because the Parameter object may be on this Parameterized instance, or it may be on the corresponding class, or it may be on some superclass. Plus the value of the Parameter isn't actually stored in the Parameter object, only the Parameterized instance. For iteration without mutation, where the actual Parameter object is stored doesn't matter, but it does matter if iteration changes where the Parameter is stored (as .object currently does by default), and it does matter if the user can modify the Parameter object during iteration (which would have wildly different impacts depending on where that Parameter object is stored, either affecting only this instance or affecting all instances of this class or other classes).

The proposal so far is to see if we can approximate normal dictionary-like behavior by providing read-only views of Parameter objects. Can we get rid of such special-purpose iterating methods as get_param_values, print_param_values, and get_param_values, and replace them with simply iteration, leaving what's done in the iteration to the user?

@jlstevens
Copy link
Contributor Author

Just to officially record something I've been saying: I think iterating over named tuples containing snapshots of parameter state would ensure immutability in a way that is obvious (and still practical) for users. Having a way to toggle immutability on Parameters would avoid needing named tuples but I'm not sure this is worth the effort (and if you do this, you would need good error messages explaining why the Parameter has been set to immutable).

@jbednar
Copy link
Member

jbednar commented Oct 20, 2021

Closing in favor of #543

@jbednar jbednar closed this as completed Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Improvement to API or something like that component: "parameterized" help me name this :) status: discussion Discussion. Not yet a specific feature/bug. Likely to result in multiple PRs/issues.
Projects
None yet
Development

No branches or pull requests

5 participants