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

R of x gets passed dimensionless x #1236

Closed
rtimms opened this issue Nov 9, 2020 · 4 comments · Fixed by #1237
Closed

R of x gets passed dimensionless x #1236

rtimms opened this issue Nov 9, 2020 · 4 comments · Fixed by #1237
Assignees
Labels
bug Something isn't working

Comments

@rtimms
Copy link
Contributor

rtimms commented Nov 9, 2020

The functions R_n_of_x etc. get passed dimensionless x but this doesn't get scaled when passed to the FunctionParameter which depends on x [m], e.g.

        inputs = {"Through-cell distance (x_n) [m]": x}
        return pybamm.FunctionParameter("Negative particle distribution in x", inputs)

should be

        inputs = {"Through-cell distance (x_n) [m]": x * self.L_x}
        return pybamm.FunctionParameter("Negative particle distribution in x", inputs)

since thicknesses are scaled by L_x

@rtimms rtimms added the bug Something isn't working label Nov 9, 2020
@rtimms rtimms self-assigned this Nov 9, 2020
@rtimms
Copy link
Contributor Author

rtimms commented Nov 9, 2020

actually, more consistent with other things (e.g. initial solid concentration) to write it as a function of dimensionless through-cell position, i.e.

        inputs = {"Dimensionless through-cell position (x_n)": x}
        return pybamm.FunctionParameter("Negative particle distribution in x", inputs)

rtimms added a commit that referenced this issue Nov 9, 2020
@valentinsulzer
Copy link
Member

Looking at this again, why do we separate the particle radius and its dependence in x into two separate parameters? Wouldn't it be clearer if it was always a single (dimensional input, dimensional output) parameter?

@rtimms
Copy link
Contributor Author

rtimms commented Nov 9, 2020

yeah that would be cleaner. i think we kept it separate so that we had a reference radius to use in the non-dimensionlisation, though i guess you just take the average for that

@valentinsulzer
Copy link
Member

Yeah, or evaluate at x=0 and x=1 (might have issues with the average not being defined until discretisation)

rtimms added a commit that referenced this issue Nov 11, 2020
rtimms added a commit that referenced this issue Nov 11, 2020
rtimms added a commit that referenced this issue Nov 11, 2020
rtimms added a commit that referenced this issue Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants