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

InterpolatedVolumeParameter documentation is confusing #889

Open
Batch21 opened this issue May 21, 2020 · 5 comments
Open

InterpolatedVolumeParameter documentation is confusing #889

Batch21 opened this issue May 21, 2020 · 5 comments

Comments

@Batch21
Copy link
Contributor

Batch21 commented May 21, 2020

The API documentation shows this:

Generic interpolation parameter calculated from current volume

init(self, model, node, x, y, interp_kwargs=None, **kwargs)
Initialize self. See help(type(self)) for accurate signature.

However if you defined the parameter in json you have to use volumes and values instead of x and y as the argument keys.

InterpolatedFlowParameter has the same issue

Easiest solution would be to add an __init__ function with correct arg names that just supers the parent class

@jetuk
Copy link
Member

jetuk commented May 21, 2020

def __init__(self, model, node, x, y, interp_kwargs=None, **kwargs):

Just rename x and y to volumes and values respectively?

Same issue here:

def __init__(self, model, node, x, y, interp_kwargs=None, **kwargs):

@Batch21
Copy link
Contributor Author

Batch21 commented May 21, 2020

Not sure what I was looking at when I thought the classes didn't have init funcs. I've update the arg names and docstrings in #890

It looks like neither parameter is tested directly though they are used in other tests?

Also it is not possible to set any of the interp_kwargs in the json other than the kind. Maybe the loads function should assume that anything left in data should be added to that input arg?

@jetuk
Copy link
Member

jetuk commented May 22, 2020

Also it is not possible to set any of the interp_kwargs in the json other than the kind. Maybe the loads function should assume that anything left in data should be added to that input arg?

I think it would be better to have interp_kwargs as a key with a sub-dictionary in the JSON. Let the __init__ method handle default arguments. This is how the schema branch does the pandas kwargs and it means top level of the JSON does not have open ended keywords.

@Batch21
Copy link
Contributor Author

Batch21 commented May 22, 2020

That is a better way of doing it. Might have to account for the kind arg not being in the interp_kwargs sub-dictionary for backwards compatibility.

@jetuk
Copy link
Member

jetuk commented May 22, 2020

#890 solved the main issue here. Shall we keep this open for the interp_kwargs issue as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants