Skip to content

Restore covariance#216

Merged
michael-petersen merged 18 commits intomainfrom
covarUpdate
Apr 22, 2026
Merged

Restore covariance#216
michael-petersen merged 18 commits intomainfrom
covarUpdate

Conversation

@The9Cat
Copy link
Copy Markdown
Member

@The9Cat The9Cat commented Apr 19, 2026

Summary

Some recent merge seems to have removed some of the inheritance tree that supported covariance functionality in pyEXP. This PR restores that.

Details

No internal logic changes or API changes here. The header files were extended/rewritten to restore the inheritance chain that allowed allowed polymorphism and dynamic dispatch in pybind11. It is possible that a pybind11 update rather than a merge error caused this, but it was hard to reconstruct the history exactly.

Checks

Verified by building a covariance file from snapshots and reading and analyzing the resulting covariance db with an existing test notebook

Notes and comments

  • The current implementation requires that the user call writeCovariance() after every coefficient build to write the covariance information to HDF5 covariance file.
  • The covariance from each snapshot is overwritten by the next coefficient build. This strategy is designed to prevent huge memory usage, possibly resulting in an out-of-memory condition.
  • The internal covariance database is not accessible with getCoefCovariance(time); that was a remnant from the first test API. User access to covariance data requires the HDF5 file to be read with basis.CovarianceReader().
  • We could consider populating the internal covariance database after each make_coefs() call, optionally, for users who want to get the covariance data using getCoefCovariance(time) during the coefficient construction loop. For example, we could assign an instance of the helper class SubsampleCovariance to Basis class and populate that during make_coef() calls and optionally by reading the HDF5 covariance file. If this is desired, maybe we should open an issue on this, rather than implementing that as part of this bug fix since it would be a significant API change?

@The9Cat The9Cat requested a review from Copilot April 19, 2026 15:57
@The9Cat The9Cat added the bug Something isn't working label Apr 19, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Restores coefficient covariance support in pyEXP by rebuilding the C++ inheritance path (moving covariance storage/API onto the Basis base) so covariance-related methods can be reached via polymorphism/dynamic dispatch in pybind11.

Changes:

  • Move covariance state and the covariance API (getCoefCovariance, writeCoefCovariance, enableCoefCovariance, etc.) from BiorthBasis into the Basis base class.
  • Update pybind11 trampolines to override the covariance virtuals and adjust bindings to re-expose covariance operations.
  • Re-point setCovarH5Compress binding to the new base-class location.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
pyEXP/BasisWrappers.cc Updates pybind11 trampolines and (re)binds covariance-related methods after the inheritance changes.
expui/BiorthBasis.H Removes covariance members/methods from BiorthBasis now that they live in Basis.
expui/BasisFactory.H Adds covariance storage/methods to the Basis base class to restore polymorphic access.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyEXP/BasisWrappers.cc
Comment thread pyEXP/BasisWrappers.cc
Comment thread expui/BasisFactory.H Outdated
Comment thread pyEXP/BasisWrappers.cc
Comment thread pyEXP/BasisWrappers.cc
Comment thread pyEXP/BasisWrappers.cc
Comment thread pyEXP/BasisWrappers.cc
Agent-Logs-Url: https://github.com/EXP-code/EXP/sessions/7396ab22-dac6-467e-9e74-f2fb2f26be91

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

pyEXP/BasisWrappers.cc:2230

  • Covariance methods are now bound only on a subset of concrete classes (e.g., Cylindrical, SphericalSL, FlatDisk, Cube). Bases like Bessel (which inherits Spherical and appears to share the same covariance-capable coefficient machinery) won’t expose getCoefCovariance / writeCoefCovariance / enableCoefCovariance in Python anymore. To restore polymorphism via the pybind inheritance tree, bind these methods once on the common base (BiorthBasis or Basis) using the virtual API, or add the missing bindings for Bessel (and any other covariance-capable derived bases).
	 py::arg("total")=true, py::arg("covar")=true);

  
    py::class_<BasisClasses::Bessel, std::shared_ptr<BasisClasses::Bessel>, BasisClasses::Spherical>(m, "Bessel")
      .def(py::init<const std::string&>(),
	 R"(

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyEXP/BasisWrappers.cc Outdated
Comment thread pyEXP/BasisWrappers.cc Outdated
Comment thread pyEXP/BasisWrappers.cc Outdated
Comment thread pyEXP/BasisWrappers.cc Outdated
Comment thread pyEXP/BasisWrappers.cc Outdated
Comment thread expui/BasisFactory.H
Comment thread pyEXP/BasisWrappers.cc Outdated
Copy tensor contents into a new numpy-owned buffer.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The9Cat and others added 5 commits April 19, 2026 12:53
copying tensor contents into a new numpy-owned buffer

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@michael-petersen
Copy link
Copy Markdown
Member

Oooops. I believe this is actually a bug against main, right? It looks like a straightforward merge against main, this would just be changing the version here to be a bug fix, i.e. 7.9.3.

@michael-petersen
Copy link
Copy Markdown
Member

I've added some test lines to make sure covariance is working; this isn't a regression test per se but it mirrors what we have been doing so far.

@The9Cat
Copy link
Copy Markdown
Member Author

The9Cat commented Apr 20, 2026

It could be a bug against main since main contains the original covariance implementation. I'll change the base and see how that goes...

@The9Cat The9Cat changed the base branch from devel to main April 20, 2026 12:17
@The9Cat
Copy link
Copy Markdown
Member Author

The9Cat commented Apr 20, 2026

Bumped the version on main to force CI.

Copy link
Copy Markdown
Member

@michael-petersen michael-petersen left a comment

Choose a reason for hiding this comment

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

Version bump + compile + tests, seems good to me, will merge this to main and release.

@michael-petersen michael-petersen merged commit e60c27c into main Apr 22, 2026
8 checks passed
@michael-petersen michael-petersen deleted the covarUpdate branch April 22, 2026 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants