Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of Bessel support in pyEXP [WIP] #75

Merged
merged 27 commits into from
May 29, 2024
Merged

Conversation

The9Cat
Copy link
Contributor

@The9Cat The9Cat commented May 13, 2024

Major changes

  • Made a spherical basis base class, Spherical, inheriting from BasisClass
  • SphericalSL and Bessel now derive from Spherical
  • Spherical contains most of the meat of the implementation. The derived classes supply the biorthogonal functions.
  • The cache requirement was moved into SphericalSL; Bessel has no cache.
  • Updated BasisWrappers to produce the new bessel type basis in the Python interface

Tests so far

  • Checked that a previous notebook that used SphericalSL (a.k.a. the sphereSL type) produced the same results
  • Generated a bessel type basis and rendered surface and line fields.
  • Checked that getBasis returned the basis functions and that they look sensible.

Martin D. Weinberg added 19 commits May 8, 2024 14:19
… added a HighFive template specification for the new API [no ci]
- Create version string variable for all HighFive-generated HDF5 caches
- This will effectively expire all existing caches
- Emit a message that the cache is rebuiling for an API change
…oose inner and outer bin edges to prevent empty bins at small and large radii [no ci]
Copy link
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.

Two quick things to understand the changes.

src/Sphere.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

What is the typical usage of noff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous implementation used the minimum and maximum particle radii to set the inner and outer bin radius, respectively. This is a bit risky because a extreme tail value could dominate the binning (esp. if logarithmic). I immediately ran into trouble when I tried it on SIDM problem. So I changed that to pick the noff point in the ordered list from each end. Or 1/2 the mean number of particles per bin if that is larger. That latter bit might be too conservative. I'm inclined to drop that last bit and set from noff`` exclusively. Then one could recover the original behavior with noff=0`. Not that the original behavior is desired.

No one, including me, is really using the recomputation strategy, so I consider it to be experimental.

Copy link
Member

Choose a reason for hiding this comment

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

Tough one to catch. Do we a) think this is the only instance, and b) how far back does this go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it goes back to the Eigen port from the Summer/Fall of 2021. I suppose it is possible that we didn't try multimass runs since. The error only is in the multimass implementation. If N were a multiple of numprocs, or if the tail of the distribution had very low probability, it still would have worked. But there's no reason to expect that to occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So to answer your question specifically, I checked that the single-mass particle selection functions do not have this issue. And they have been used multiple times recently without issue. That doesn't mean that there isn't some other fence-post issue in there somewhere.

Martin D. Weinberg added 3 commits May 17, 2024 17:52
… mistake in default value for Coef::EvenOddPower() [no ci]
…kward compatibility with the old (buggy) HighFive Eigen wrappers
@The9Cat
Copy link
Contributor Author

The9Cat commented May 22, 2024

This last commit implements versioning for Coef HDF files.

  • This is likely to have additional uses moving forward. The current version tag is the string "1.0".
  • For now, the existence of the version tag is used to select backward compatibility for handing the HighFive Eigen bug. If the version field is not present backward-compatibility mode is selected automatically.
  • I've also included a manual override for pyEXP processing; specifically, one can request that coefficient reads ignore backward compatibility even if the version field is not present. This can be removed in the very near future but is needed for any work done using the current development tree.

Copy link
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.

Some suggestions on backwards compatibility changes.

@The9Cat
Copy link
Contributor Author

The9Cat commented May 23, 2024

  • Check that exp produces the expected coefficient files with the latest versioning addition.
  • Check that versioning attribute is correctly toggling the backward-compatibility implementation.

@michael-petersen
Copy link
Member

I've been using this as my local branch and haven't detected any changes. Merging back now.

@michael-petersen michael-petersen merged commit 6e46751 into main May 29, 2024
0 of 8 checks passed
@michael-petersen michael-petersen deleted the pyEXPbessel branch May 29, 2024 15:38
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