-
Notifications
You must be signed in to change notification settings - Fork 104
Add Redl model of bootstrap current #1852
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
base: main
Are you sure you want to change the base?
Conversation
590651e to
6378de2
Compare
|
@jcitrin as this is really an extension of the Sauter model, would you like it as a separate class, subclass, or flag within Sauter? |
|
Excellent, thanks for this. It would be good to have redl as a separate class, even if it's quite similar to sauter, and ensure that "sauter" and "redl" can be distinct model names. Possibly a lot from both can be separated out to use the common formulas module and duplication minimized, as you say. It may be good to do this in more than 1 PR, separating out refactoring and new feature. lmk when review is necessary and we can get back to this in the new year. Have a great holiday period! |
|
I suggest the following design:
neoclassical.formulas.shared._calculate_analytic_bootstrap_current(
*,
bootstrap_multiplier: float,
n_e: cell_variable.CellVariable,
n_i: cell_variable.CellVariable,
T_e: cell_variable.CellVariable,
T_i: cell_variable.CellVariable,
p_e: cell_variable.CellVariable,
p_i: cell_variable.CellVariable,
geo: geometry_lib.Geometry,
L31: array_typing.FloatVectorFace,
L32: array_typing.FloatVectorFace,
L34: array_typing.FloatVectorFace,
alpha: array_typing.FloatVectorFace,
) -> base.BootstrapCurrent
neoclassical.bootstrap.sauter.calculate_bootstrap_terms(...) -> ...:
f_trap = neoclassical.formulas.sauter.calculate_ftrap(...)
log_lambda_ei = collisions.calculate_log_lambda_ei(...)
log_lambda_ii = collisions.calculate_log_lambda_ii(...)
nu_e_star = collisions.calculate_nu_e_star(...)
nu_e_star = collisions.calculate_nu_i_star(...)
L31 = neoclassical.formulas.sauter.calculate_L31(...)
L32 = neoclassical.formulas.sauter.calculate_L32(...)
L34 = neoclassical.formulas.sauter.calculate_L34(...)
alpha = neoclassical.formulas.sauter.calculate_alpha(...)
return L31, L32, L34, alphaI think this is the most simple option that minimises duplication. Also I think this structure sets up Andreas' intent to do the new conductivity model. @jcitrin thoughts? |
|
@theo-brown this structure sounds good to me. @tamaranorman , @Nush395 , any thoughts on the proposed structure? Context: Theo and Andreas Redl are implementing the Redl model for bootstrap current and conductivity, which is an upgrade on the sauter model. Structurally it's very similar to the sauter models, but have slightly different formulas and coefficients. Theo's proposal aims to minimize the duplication and still have both |
| from absl.testing import parameterized | ||
| import numpy as np | ||
| import xarray as xr | ||
| from absl.testing import absltest, parameterized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert these changes the google style guide recommend everything be imported per module excluding types or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a unit test on the numerics of this file etc instead of just testing through integration tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this if its also tested as part of the step case? Just that as we add more and more of these the time taken for tests change - it would be cleaner if this could be tested in a unit test
|
Looks good in its current form - one question is when we talk about rearranging what does that look like as this currently is just adding another implementation |
|
Thanks for your review @tamaranorman! I was mainly @'ing you for your thoughts on the comment, sorry. See my comment above for my proposed structure (link) - I didn't want to waste time implementing before a design had been chosen. The "changes to structure" are just moving things around, rather than introducing any new classes etc. I think Jonathan was just to check whether there's an alternative (eg create a new superclass that both of these inherit from) or whether having this level of code duplication is ok/acceptable/avoidable. |


Prompted by JINTRAC NCLASS comparison of Sauter and Redl models, this has been bumped up my priority list!
When I have time, I'll try and rearrange some of the definitions and files so there's less duplication.
Closes #1612.