-
Notifications
You must be signed in to change notification settings - Fork 113
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 |
|
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. |
|
That proposed structure seems reasonable to me - maybe returning a dict that can then be unpacked using ** just as with lots of scalars its worth making sure the return values are clear |
d3b0a7b to
ea3df38
Compare
ea3df38 to
2efbae3
Compare
d7a137a to
e770b00
Compare
|
Thanks for the review @tamaranorman. Sorry that I still haven't managed to make my linter sync up with the Google one! |
001f3dc to
28a8d0f
Compare
- As part of this PR, we've restructured the neoclassical package to include separate modules for formulae from different analytical models. This sets up future PR for implementing the Redl model of conductivity. - Added a function for common functionality between Redl and Sauter. - Unit tests for Redl model.
28a8d0f to
f72e59c
Compare
-- 001f3dc by Theo Brown <7982453+theo-brown@users.noreply.github.com>: Add Redl model of bootstrap current Includes restructure of the neoclassical package to include separate modules for formulae from different analytical models. This sets up future PRs for implementing the Redl model of conductivity. COPYBARA_INTEGRATE_REVIEW=#1852 from google-deepmind:redl-bootstrap 001f3dc PiperOrigin-RevId: 862696005
|
This has been merged, I just need to fix the copybara error. |


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.