Skip to content

Conversation

The9Cat
Copy link
Member

@The9Cat The9Cat commented May 10, 2025

These changes result from tracking down weirdness in the pyEXP Zang disk velocity test

  • Set the basis limits from a density input file used to construct orthogonal polynomials. This was the main reason that the Zang disk example was failing. So not really a bug per se, bad a very bad, confusing feature.
  • Remove the weight field from the field return. This is the same as density so both redundant and confusing.
  • Allow the user to use mmax instead of lmax for cylindrical field coefficients.
  • Remove the default linear mapping from the OrthoFunction class in favor of the usual coodrdinate mapping set by the rmapping key.
  • Thoroughly checked the accuracy of the OrthoFunction for an exponential density weight, demonstrating the recovery of the generalized Laguerre polynomials to high accuracy (e.g. 1 in $10^{12}$).

To be done

  • Recheck Zang example
  • Recheck DiskHalo example
  • Generate some new examples?

Martin D. Weinberg added 8 commits May 5, 2025 11:31
…ordinate transformation which works way better for power laws and exponentials
…mass; this doesn't affect any computation in EXP so far
…' field sufficies'; (2) fix a shadowing error in the density file construction that did not set the class version of 'rmin' and 'rmax' from the file resulting in a poor basis construction if 'rmin' and 'rmax' were *not* set in the configuration file as well.
@michael-petersen michael-petersen requested a review from Copilot May 10, 2025 13:28
Copy link
Contributor

@Copilot 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

This PR fixes a failing Zang disk velocity test by updating the basis limits, removes redundant "weight" fields, and introduces support for using "mmax" in cylindrical field coefficient calculations. Key changes include:

  • Removal of obsolete keys in OutVel configuration.
  • Updates to the OrthoFunction interface and internal usage following the removal of the "segment" parameter.
  • Adjustments in integration and coefficient assignment in Mestel disk and FieldBasis routines.

Reviewed Changes

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

Show a summary per file
File Description
src/OutVel.cc Removes obsolete keys from configuration before constructing basis.
pyEXP/CoefWrappers.cc Updates docstring to reflect cylindrical coefficient output.
include/OrthoFunction.H Removes the 'segment' parameter and associated member variable.
exputil/mestel.cc Adjusts integration weighting to improve mass calculation accuracy.
exputil/OrthoFunction.cc Revises indexing and removes the use of the 'segment' parameter.
expui/FieldBasis.cc Streamlines key processing, updates coefficient dimensions, and refines spherical evaluation.
Comments suppressed due to low confidence (2)

exputil/mestel.cc:161

  • The integration formula has been altered by weighting 'lst' with (rr-dr) instead of rr. Please verify that this change correctly reflects the intended radial integration for the disk mass calculation.
cum += 0.5*dr*(lst*(rr-dr) + cur*rr) * 2.0*M_PI;

expui/FieldBasis.cc:460

  • The change from using 'rsqrt(fabs(1.0 - costhcosth))' to 'r' as input for poly_eval in the spherical evaluation may affect the computed coefficients. Please confirm that this adjustment is intentional.
auto p = (*ortho)(r);

@The9Cat
Copy link
Member Author

The9Cat commented May 12, 2025

New updates include:

  • Ensuring that pybind11 gets copies not maps to coefficient data stores, enforcing that all changes be made with the setMatrix (and etc.) routines rather than using the mapped object, which is confusing
  • Ensured that generated OrthoFunction instances return rank nmax vectors
  • Additional comments
  • Checked previous velocity field tests and included a new disk-halo type test based on a simulation with much larger N. See Some velocity field examples [WIP] pyEXP-examples#14

To Do

Would be really useful for someone besides me to try this stuff...I'm still a bit nervous after the last go around that issues still lurk.

@michael-petersen
Copy link
Member

Looks good to me. I fixed one typo in the comments and suppressed one warning in oftest.cc.

I also bumped the version. This looks good to go to me.

Copy link
Member Author

@The9Cat The9Cat left a comment

Choose a reason for hiding this comment

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

Thanks. Yes, the base class (gen2d in this case) should have a virtual destructor, not that it would matter much for this case. How did you catch that??

@The9Cat
Copy link
Member Author

The9Cat commented May 13, 2025

I'll do the merge. We can sort additional issues later, if they arise. After all, this is a really a bug fix.

@The9Cat The9Cat merged commit ce347f5 into main May 13, 2025
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.

2 participants