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

Make y, z optional arguments forFunctionParameters that depend on space #3631

Open
rtimms opened this issue Dec 18, 2023 · 5 comments · May be fixed by #4883
Open

Make y, z optional arguments forFunctionParameters that depend on space #3631

rtimms opened this issue Dec 18, 2023 · 5 comments · May be fixed by #4883
Assignees
Labels
priority: medium To be resolved if time allows

Comments

@rtimms
Copy link
Contributor

rtimms commented Dec 18, 2023

Some FunctionParameters depend on space and time (e.g. ambient temperature) but it doesn't always make sense to allow these to depend on space, for example if the thermal model is lumped then the temperature cannot depend on space. For the user, this can be confusing (see #3630).

Is there a nice way to be able to define these functions differently depending on the model options? Then for lumped models users can pass T(t) and if they try to pass T(y,z,t) we can catch it and throw an informative error.

@rtimms rtimms added the priority: medium To be resolved if time allows label Dec 18, 2023
@rtimms
Copy link
Contributor Author

rtimms commented Dec 18, 2023

From @ejfdickinson

I'd feel it's most Pythonic to put optional arguments last, and explicitly allow them to be optional. In general we don't want to have to write functions whose return values don't depend on their arguments, right?

What's happened here is that you've enabled complicated behaviour by requiring simple behaviour to have a complicated calling pattern!

Yes, this should work. Let's do that

@rtimms rtimms changed the title Define FunctionParameters that depend on space differently depending on geometry option Make y, z optional arguments forFunctionParameters that depend on space Dec 18, 2023
@rtimms
Copy link
Contributor Author

rtimms commented Dec 18, 2023

In the actual model definitions, we will need to update the calls from (y,z,t) to (t, y=y, z=z) (best to be explicit in the latter in anticipation of other coordinate systems).

@cringeyburger
Copy link
Contributor

Hi! I would like to take this on.

@cringeyburger
Copy link
Contributor

From what I understand, we need to change the calls from (y, z, t) to (t, y=y_1, z=z_1), where y_1 and z_1 are two default values, and we need to change this call in the actual model definition, not JUST in thermal_parameters.py? (talking about T_amb method)
But what are the default values y_1 and z_1 when the model is in thermal equilibrium or lumped? The model doesn't depend on distance parameters, right? (I don't know much about batteries, so that I may be misunderstanding something)

@Rishab87
Copy link
Contributor

I would like to work on this issue, can this be assigned to me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium To be resolved if time allows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants