Skip to content

BiorthBasis::Cylinder deprojection improvements#208

Open
The9Cat wants to merge 81 commits intodevelfrom
Toomre
Open

BiorthBasis::Cylinder deprojection improvements#208
The9Cat wants to merge 81 commits intodevelfrom
Toomre

Conversation

@The9Cat
Copy link
Member

@The9Cat The9Cat commented Mar 13, 2026

Movitation

Needed a less cuspy option than exponential for attempting to produce a doughnut-shaped basis. So this adds a Toomre model for that purpose.

Improvements

  • I noticed that the numerical Abel transform had poor systematics in the inner profile. This is not a bug fix, but it is a noticeable improvement. The original version would not cause failure but could result in weaker EOF convergece.
  • Fixed a bug in Cylindrical parameter parsing: the dmodel key was read as a bool rather than a string.
  • I added all three numerical variants as source configuration choices and switched to the internal derivative form by default. I provided some verification and test routines for this in utils/Tests. These methods could be made user selectable but I have not done this in this PR.
  • I added a Python functor for the disk profile used in the spherical deprojection; this adds more generality for non-traditional surface density profiles.
  • A standalone test driver named testEmp that uses the same algorithm as EmpCylSL is included in utils/Test. I also provided two basic sanity checks on Abel transforms for verification and debugging. These could be removed.

Notes and comments

  • Trying to make an exponential profile with an inner cutoff (doughnut) with a an exponential deprojection results in ringing features in the doughnut hole
  • A more gradual surface density with no cusp for the deprojection, such as a Toomre surface density, helps reduce the wiggle features a bit. Only a bit because the inner roll over needs to be very shallow for positive density.
  • One can not use a basis with a sharp inner cut because this yields negative density in the deprojection.
  • It is additionally helpful, although not strictly necessary, to roll over the inner density for the basis construction more slowly than the target density to keep the Abel deprojected density positive.

Examples

  • First 8 basis functions for m=0 for a exponential disk with h/a=0.1, a=1 and a $(1+\tanh([R-0.005]/0.001)$ inner hole. The target basis is the deprojected disk with $\Sigma=(1+R^2)^{-3/2}$ with the inner rollover $(1+\tanh([R-0.005]/0.02)$, the smallest width that gives positive deprojected density. The inner edge of the model is obvious in these images.
exp4_m2 - First 8 basis functions as above but for a deprojected exponential basis with no hole. It is not too terrible and the orthogonality remains excellent, but there is more noise inside the cutoff for this one. exp1_m2

@The9Cat The9Cat requested a review from Copilot March 13, 2026 16:26
@The9Cat The9Cat changed the title Toomre BiorthBasis::Cylinder deprojection improvements Mar 13, 2026
Copy link
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

Adds a Toomre disk option for cylindrical-basis deprojection/conditioning, expands the Abel inversion implementations used during deprojection, and introduces several standalone test/verification executables under utils/Test.

Changes:

  • Add a new Toomre disk model and wire a configurable deprojection model selector (dmodel) into Cylindrical basis initialization.
  • Update EmpCylSL::create_deprojection to support multiple Abel inversion variants (Derivative/Subtraction/IBP) with derivative as default.
  • Add Deprojector/EmpDeproj test utilities and build targets in utils/Test.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 16 comments.

Show a summary per file
File Description
utils/Test/testEmpDeproj.cc New CLI test comparing EmpDeproj vs Deprojector across profiles/methods
utils/Test/testDeproject.cc New example program for Deprojector (sampled + analytic construction)
utils/Test/testDeprojPlummer.cc New simple deprojection verification driver (currently hardwired)
utils/Test/EmpDeproj.cc New empirical deprojection implementation used by test programs
utils/Test/EmpDeproj.H Header for EmpDeproj test-class API
utils/Test/Deprojector.cc New numerical Abel deprojection implementation and grid integration logic
utils/Test/Deprojector.H Header/API for Deprojector
utils/Test/CubicSpline.cc New spline helper for sampled-data deprojection path
utils/Test/CubicSpline.H Header for CubicSpline
utils/Test/CMakeLists.txt Adds new test executables to the build
include/EmpCylSL.H Adds AbelType + abel_type member to control deprojection variant
include/DiskModels.H Adds Toomre disk density model for deprojection conditioning
exputil/EmpCylSL.cc Switchable Abel inversion variants used in deprojection pipeline
expui/BiorthBasis.cc Adds dmodel config key + lookup map and uses it when deproject is enabled
expui/BiorthBasis.H Introduces DeprojType enum + lookup map for deprojection disk models

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

You can also share your feedback on Copilot code review. Take the survey.

The9Cat and others added 8 commits March 13, 2026 12:42
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>
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>
Copy link
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

Adds a Toomre-based option for cylindrical deprojection and refines the numerical Abel inversion machinery, along with several small test/verification executables under utils/Test.

Changes:

  • Add a new Toomre disk model for deprojection and wire it into cylindrical basis initialization via a dmodel lookup.
  • Introduce multiple Abel inversion variants (derivative/subtraction/IBP) in the EmpCylSL deprojection path (defaulting to derivative).
  • Add standalone utils/Test deprojection utilities (Deprojector, EmpDeproj) and test drivers, and hook them into utils/Test/CMakeLists.txt.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
utils/Test/testEmpDeproj.cc New CLI test comparing EmpDeproj against Deprojector for several analytic profiles.
utils/Test/testEmp.cc New CLI test for donut-shaped (inner cut) empirical deprojection.
utils/Test/testDeproject.cc New minimal examples for Deprojector from sampled data vs functor.
utils/Test/testDeprojPlummer.cc New Plummer/Gaussian/Toomre Deprojector validation driver.
utils/Test/EmpDeproj.cc Adds an empirical deprojection implementation used by the new test drivers.
utils/Test/EmpDeproj.H Declares the EmpDeproj helper class and AbelType selection.
utils/Test/Deprojector.cc Adds a standalone deprojection implementation with tail-matching and improved inner integration handling.
utils/Test/Deprojector.H Declares the standalone Deprojector API used by tests.
utils/Test/CubicSpline.cc Adds a basic natural cubic spline for sampled-data deprojection.
utils/Test/CubicSpline.H Declares the cubic spline used by Deprojector sampled-data ctor.
utils/Test/CMakeLists.txt Registers the new utils/Test executables in the build.
include/EmpCylSL.H Adds AbelType state to EmpCylSL for selecting deprojection variant.
include/DiskModels.H Adds a new Toomre disk density model (used for deprojection conditioning).
exputil/EmpCylSL.cc Updates EmpCylSL’s Abel inversion integral to support multiple variants.
expui/BiorthBasis.cc Adds dmodel string parsing + lookup for selecting deprojection model (mn/exponential/toomre/python).
expui/BiorthBasis.H Adds DeprojType and makes DiskType an enum class for stronger typing.

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

You can also share your feedback on Copilot code review. Take the survey.

The9Cat and others added 10 commits March 13, 2026 14:32
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>
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>
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>
@The9Cat
Copy link
Member Author

The9Cat commented Mar 24, 2026

Updates to caching and Cylinder

This set of commits implements stateful caching for the disk targets and projection types. This implements the full set of target densities currently available in BiorthBasis::Cylindrical in the N-body Cylinder. I did not implement deprojection in Cylinder but it would not be hard. This update implements the suggestions in the previous comment above.

I tagged the branch before merging these changes with the tag preCache. This allows you to check out the latest commit before all of this caching stuff. Although it would be helpful to try all of this stuff, if you can.

Some of the control interface between BiorthBasis::Cylindrical and EmpCylSL was confused by the addition of the additional deprojection densities including the the Python functor. This set of commits cleans that up. The alternative deprojection densities are not yet ported to Cylinder.

Change summary and behavior description

On construction:

  • Save the target disk type in the cylindrical basis cache
  • If the target density is implemented by Python module, cache the md5sum of the module source
  • Save the deprojection type in the cylindrical basis cache
  • If the surface density for deprojection is implemented by a Python module, cache the md5sum

On read:

  • If the target disk type is not found in the cache, an old cache is assumed and allowed with no further checking
  • If the DiskType is found, it must match the requested type, otherwise we trigger a cache rebuild
  • If the DiskType is Python, then the md5 hash must match the md5 hash of the supplied module, otherwise we trigger a cache rebuild
  • If deproject is true, then the deprojection model type (dmodel) must match the requested one, or we trigger a cache rebuild
  • If dmodel is Python, then the md5 hash must match the md5 hash of the supplied module, otherwise we trigger a cache rebuild

For exp N-body:

All of the above applies except for the deproject stuff, which is not (yet) implemented

Testing so far

I have confirmed that:

  1. Python and non-Python bases can be made with the expected meta-data in the cache file for BiorthBasis::Cylindrical
  2. The cache logic acts as expected when using an old basis, or using a basis with the wrong target parameters
  3. The cache is corrected reread and the basis is loaded when cache parameters agree
  4. Metadata in HDF5 files have been inspected for consistency and readability
  5. EXP generates and, on restart, correctly verifies and uses the new cache
  6. EXP can read and use a standard exponential basis created by pyEXP. I have not tested all combinations of spherical models.

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.

5 participants