Skip to content

Added memory estimates in struphy and feectools - #317

Draft
max-models wants to merge 15 commits into
develfrom
316-allow-to-estimate-memory-usage-before-allocate
Draft

Added memory estimates in struphy and feectools#317
max-models wants to merge 15 commits into
develfrom
316-allow-to-estimate-memory-usage-before-allocate

Conversation

@max-models

@max-models max-models commented Aug 3, 2026

Copy link
Copy Markdown
Member

Corresponding PR in feectools: struphy-hub/feectools#75

@max-models max-models linked an issue Aug 3, 2026 that may be closed by this pull request
@max-models
max-models requested a review from Copilot August 3, 2026 12:55
@max-models
max-models marked this pull request as draft August 3, 2026 12:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_run flag and an nbytes_local estimator to Particles to 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.

Comment thread src/struphy/simulation/sim.py
Comment thread src/struphy/pic/base.py
Comment thread src/struphy/models/variables.py
Comment thread src/struphy/simulation/sim.py
Comment thread src/struphy/simulation/sim.py
max-models and others added 3 commits August 3, 2026 15:07
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>

@spossann spossann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/struphy/pic/base.py Outdated

nbytes = 0
nbytes += n_rows * n_cols * float_size # markers
nbytes += n_rows * n_cols * float_size # sorting_etas (mpi_sort_markers buffer)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course! This is the priority for today!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 44242cf

@max-models max-models changed the title Added memory estimates for the Variable classes using a dry_run flag Added memory estimates in struphy and feectools Aug 4, 2026
@max-models

Copy link
Copy Markdown
Member Author

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_

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow to estimate memory usage before allocate

4 participants