Skip to content

Depletion module code improvements #3349

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

Open
yardasol opened this issue Mar 13, 2025 · 0 comments
Open

Depletion module code improvements #3349

yardasol opened this issue Mar 13, 2025 · 0 comments

Comments

@yardasol
Copy link
Contributor

yardasol commented Mar 13, 2025

Description

This issue contains several small proposals to improve source code readability in the depletion module.

  1. Some key internal functions in openmc.deplete do not have docstrings:
  • Integrator._timed_deplete()
  • SIIntegrator._get_bos_data_from_operator()
  • Integrator._get_start_data()
    Docstrings should be added to these functions.
  1. Some classes/functions could benefit from different internal variable names:
  • Integrator._i_res. Maybe step_index
  • p used instead of source_rate in SIIntegrator.integrate()
  • x, n, bos_conc used interchangeably for the nuclide concentration in Integrator.integrate(), SIIntegrator.integrate(), Integrator._get_bos_data_from_operator(), etc. This also applies to n_list. We should pick one name and use it consistently across the entire codebase. I suggest concentration
  • res, res_list in the same functions as the above item. I suggest just expanding this to result.
  • n in deplete()
  • func in deplete() to something more descriptive (this function solves the depletion matrix).
    I'd love to hear other's opinions on the naming scheme.
  1. Rename the __call__() class methods to something more readable
  • Integrator.__call__() to something like Integrator.integrate_single_step()
  • IPFCramSolver.__call__() to something like IPFCramSolver.solve()
  • TransportOperator.__call__() to something like TransportOperator.simulate()
  1. Collapse DepSystemSolver into IPFCramSolver
  • DepSystemSolver is an abstract class with a single abstract method, and only has a single child class (IPFCramSolver). I propose removing DepSystemSolver unless the intention is to build it out to enable other kinds of depletion matrix solvers.
  1. Remove the matric_func parameter in deplete()
  • My understanding is that we exclusively use the Chain.form_matrix() function for this. Are there cases where we want to allow a user to write their own function to build the matrix?

Alternatives

Compatibility

This may require some changes to docstrings depending on what action is taken.

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

No branches or pull requests

1 participant