Skip to content

Conversation

@theo-brown
Copy link
Collaborator

@theo-brown theo-brown commented Dec 15, 2025

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.

@theo-brown
Copy link
Collaborator Author

theo-brown commented Dec 15, 2025

STEP (dashed Redl, solid Sauter)

Figure_1

ITER (dashed Redl, solid Sauter)

Figure_1

@theo-brown
Copy link
Collaborator Author

@jcitrin as this is really an extension of the Sauter model, would you like it as a separate class, subclass, or flag within Sauter?

@jcitrin
Copy link
Collaborator

jcitrin commented Dec 24, 2025

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!

@theo-brown
Copy link
Collaborator Author

theo-brown commented Dec 30, 2025

I suggest the following design:

  1. Move neoclassical.formulas to a subpackage, so that we can have neoclassical.formulas.shared, neoclassical.formulas.sauter and neoclassical.formulas.redl.

  2. Extract the shared code from

    # calculate bootstrap current
    prefactor = -geo.F_face * bootstrap_multiplier * 2 * jnp.pi / geo.B_0
    pe = p_e.face_value()
    pi = p_i.face_value()
    dpsi_drnorm = psi.face_grad()
    dlnne_drnorm = n_e.face_grad() / n_e.face_value()
    dlnni_drnorm = n_i.face_grad() / n_i.face_value()
    dlnte_drnorm = T_e.face_grad() / T_e.face_value()
    dlnti_drnorm = T_i.face_grad() / T_i.face_value()
    global_coeff = prefactor[1:] / dpsi_drnorm[1:]
    global_coeff = jnp.concatenate([jnp.zeros(1), global_coeff])
    necoeff = L31 * pe
    nicoeff = L31 * pi
    tecoeff = (L31 + L32) * pe
    ticoeff = (L31 + alpha * L34) * pi
    j_parallel_bootstrap_face = global_coeff * (
    necoeff * dlnne_drnorm
    + nicoeff * dlnni_drnorm
    + tecoeff * dlnte_drnorm
    + ticoeff * dlnti_drnorm
    )
    j_parallel_bootstrap = geometry_lib.face_to_cell(j_parallel_bootstrap_face)
    return base.BootstrapCurrent(
    j_parallel_bootstrap=j_parallel_bootstrap,
    j_parallel_bootstrap_face=j_parallel_bootstrap_face,
    )

    into a new function

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
  1. Make a new function for each of Sauter and Redl that computes the terms that are different, e.g.
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, alpha

I 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?

@jcitrin
Copy link
Collaborator

jcitrin commented Jan 2, 2026

@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 sauter and redl as model types.

@theo-brown theo-brown requested review from Nush395 and tamaranorman and removed request for tamaranorman January 5, 2026 09:46
@theo-brown theo-brown added the ready for review Add to PRs when they are ready for review unsure of who to assign as reviewer label Jan 5, 2026
@tamaranorman
Copy link
Member

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

@theo-brown
Copy link
Collaborator Author

theo-brown commented Jan 5, 2026

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.

@theo-brown theo-brown removed the ready for review Add to PRs when they are ready for review unsure of who to assign as reviewer label Jan 5, 2026
@tamaranorman
Copy link
Member

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

@theo-brown theo-brown force-pushed the redl-bootstrap branch 6 times, most recently from d3b0a7b to ea3df38 Compare January 19, 2026 22:01
@theo-brown theo-brown added the ready for review Add to PRs when they are ready for review unsure of who to assign as reviewer label Jan 19, 2026
@theo-brown theo-brown force-pushed the redl-bootstrap branch 2 times, most recently from d7a137a to e770b00 Compare January 20, 2026 12:07
@theo-brown
Copy link
Collaborator Author

Thanks for the review @tamaranorman. Sorry that I still haven't managed to make my linter sync up with the Google one!

@theo-brown theo-brown added copybara:import-manual Set when ready for copybara manual import and removed ready for review Add to PRs when they are ready for review unsure of who to assign as reviewer labels Jan 23, 2026
@theo-brown theo-brown force-pushed the redl-bootstrap branch 2 times, most recently from 001f3dc to 28a8d0f Compare January 29, 2026 08:12
- 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.
copybara-service bot pushed a commit that referenced this pull request Jan 29, 2026
--
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
@theo-brown
Copy link
Collaborator Author

This has been merged, I just need to fix the copybara error.

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

Labels

copybara:import-manual Set when ready for copybara manual import

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Redl model of bootstrap current

3 participants