Added memory estimates in struphy and feectools - #317
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a “pre-allocation” memory estimation path for model variables by adding estimate_mem() APIs across variable types and enabling a dry_run sizing mode for particle storage, so users can gauge memory needs before calling Simulation.allocate().
Changes:
- Added
Simulation.estimate_mem()to build a lightweight Derham instance and aggregate per-variable memory estimates. - Introduced
Variable.estimate_mem()(abstract) and implementations for FEEC/PIC/SPH variables, including a coefficient-space sizing helper. - Added a
dry_runflag and annbytes_localestimator toParticlesto compute marker-array sizing without allocating large arrays.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/struphy/simulation/sim.py |
Adds Simulation.estimate_mem() and reporting of per-variable and total memory estimates. |
src/struphy/pic/base.py |
Adds dry_run initialization mode and nbytes_local memory estimation for particle marker-related arrays. |
src/struphy/models/variables.py |
Adds an abstract estimate_mem() API and concrete memory estimators for FEEC/PIC/SPH variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
That looks good so far. However, there is a big piece missing: the FEEC matrices, which are way bigger than the vectors! The best way to track them is I guess within StencilMatrix.__init__(), which could be augmented with a dry_run argument, returning before self._data = np.zeros(dims+diags, dtype=W.dtype) is called.
|
|
||
| nbytes = 0 | ||
| nbytes += n_rows * n_cols * float_size # markers | ||
| nbytes += n_rows * n_cols * float_size # sorting_etas (mpi_sort_markers buffer) |
There was a problem hiding this comment.
There is an inefficiency uncovered: sorting_etas can be of shape (self.markers.shape[0], 3), just as is_on_proc_domain - please adapt in the code and see if tests still pass.
There was a problem hiding this comment.
Yes, of course! This is the priority for today!
|
In the workflows, we check the submodule diffs only after installing struphy. However, in this PR, the struphy installation fails because it relies on the feectools version in the submodule, see https://github.com/struphy-hub/struphy/actions/runs/30913232722/job/92004850136?pr=317#step:9:986 for example. It would be nice to change the order so we install feectools first, but I think submodule-diff currently depends on the install step because the current branch is fetched and checked out in struphy_in_container, what do you think could be a solution? @spossann - name: Install Struphy in Container
uses: ./.github/actions/install/struphy_in_container
- name: Get submodule diff
uses: ./.github/actions/submodule-diff
with:
start-dir: /struphy_fortran_
- name: Reinstall feectools from submodule
if: env.SUBMOD_CHANGED == 'true'
uses: ./.github/actions/install/feectools-submodule
with:
env-name: /struphy_fortran_/env_fortran_ |
Corresponding PR in feectools: struphy-hub/feectools#75