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

Minor bug fixes and updates #101

Merged
merged 94 commits into from
Dec 28, 2024
Merged

Minor bug fixes and updates #101

merged 94 commits into from
Dec 28, 2024

Conversation

The9Cat
Copy link
Contributor

@The9Cat The9Cat commented Dec 21, 2024

This branch is a devel with a bunch of minor updates and feature enhancements as follows:

  • Updates to Eigen support for API tweaks to esp. Ref mapping in version 3.4
  • Fixes for HDF5 API changes
  • Improvements to CMake platform and compiler support in the top-level CMakeLists.txt and in specific package support for some of the specific libraries (e.g. exputil, expui and src).
  • Added git detection to top-level CMakeLists.txt
  • Made the my_err debugging variable static in fpetrap.h, mostly to prevent multiple stack allocations which makes the address sanitizer noisy.
  • Fixed a bug in the CUDA detection that decides whether to get particle data from the GPU device in ExternalCollection
  • Provide an initial value modified=0 in Component. Previously, this value could be uninitialized on instantiation.
  • Fixes and enhancements to the FlatDisk implementation that includes Zang disk support. This fixes errors in the implementation that is currently in main. The main blunder is a normalization error that crept in with the FlatDisk class a while back.
  • Standalone VTK file support that allows *.vtr files to be written without dependence on the VTK package. While I've tried to keep EXP consistent with a system VTK install, it's increasingly hard to get it right.
  • A new FieldGenerator spherical histogram routine called histo1dlog that has a logarithmic radial spacing. Motivated by measuring halo profiles
  • The new draft pseudo force support controlled by the nEJaccel parameter. The default is the original behavior.

Martin D. Weinberg and others added 30 commits July 7, 2024 11:39
@The9Cat
Copy link
Contributor Author

The9Cat commented Dec 21, 2024

  • Compiles with Clang (18.1 tested on Linux)

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.

All good updates, and no apparent breaking changes. Everything compiles, so I'm willing to say this is ready for a merge to main so that we can branch back off from it. As usual, documentation is going to be the most annoying to-do from this.

CMakeLists.txt Show resolved Hide resolved
expui/BasisFactory.cc Show resolved Hide resolved
expui/BiorthBasis.H Show resolved Hide resolved
expui/BiorthBasis.cc Show resolved Hide resolved
expui/BiorthBasis.cc Show resolved Hide resolved
src/ComponentContainer.cc Show resolved Hide resolved
src/Cylinder.cc Outdated Show resolved Hide resolved
src/cudaPolarBasis.cu Show resolved Hide resolved
src/user/UserHalo.cc Show resolved Hide resolved
utils/ICs/initial2d.cc Show resolved Hide resolved
@michael-petersen
Copy link
Member

I should say that while I did leave a few questions throughout that would be useful to answer, that doesn't stop me thinking this is ready for merge.

@The9Cat
Copy link
Contributor Author

The9Cat commented Dec 21, 2024

We can mull over what makes the most sense for the CBDisk before merging. I can go either way. Mostly my thinking goes: this is one of the historical, published bases, so a BFE package seems like it should have it available. Ditto on CBSphere. On the other hand, I don't think it's very useful for modern research so perhaps it's not worth having more code to maintain. And to be fair, we've dumped the 3d Clutton-Brock basis.

@michael-petersen
Copy link
Member

Just caught a couple more AddAcc->AddAccExt changes.

On CBDisk, I'm torn. I agree that as a BFE code, it's a bit sully not to have the classic sets available. At the same time, we are in principle offering equivalent (or better) products through the standard EXP solutions. This is certainly true in the spherical cases, where the tabulated results are more stable than recursions, but as you point out, the solution off the disc may be worse in the 2d case when using a Hankel transform.

To me, it comes down to which principle we want to follow: is it providing the best numerical solution regardless of method, or is it producing the EXP-like solutions (i.e. numerical tables)? This choice (if we make one) might also help the doc writing for suggestions of when to use methods.

@The9Cat
Copy link
Contributor Author

The9Cat commented Dec 22, 2024

Good points.

My current POV is that we should ax stuff that we believe shouldn't be used either because the method isn't up to the task or it will lead to confusion.

CBDisk is a particularly ambiguous case. One can get very close to the CBBasis provided by the recursion relations by using FlatDisk with a density from the Plummer-like base pair. It's not identical because it uses EOF not an exact (Hankel) transform. Of course, the goal of FlatDisk is provide the vertical forces as well (from the EOF basis using a numerical Hankel transform). So it sort of supercedes CBDisk. Also, CBDisk has not been widely used by any of us so perhaps that's a strike against it. That said, I used it recent for numerical comparison but I can't imagine choosing it for a real simulation project. And I didn't really simulate anything with it...

@The9Cat
Copy link
Contributor Author

The9Cat commented Dec 22, 2024

Here's a suggestion apropos CBDisk: implement a preprocesor flag that optionally compiles and exposes the Clutton-Brock disk. This could be CMake variable (e.g. ENABLE_CBDISK) or maybe that's too visible for a possibly temporary feature?

Martin D. Weinberg added 5 commits December 23, 2024 12:28
- parallels the method for computing the center motion
- enhances the PseudoAccel class
- adds a new member function to Component needed for Coriolis, Euler, and Centrifugal forces
- makes the intent clear when we get to testing the algorithm
…be computed and recorded in the orient log file
@michael-petersen
Copy link
Member

I think it makes sense to merge this back into main ahead of other PRs, once you are happy that PseudoAccel work is at a reasonable resting place, @The9Cat ?

@The9Cat
Copy link
Contributor Author

The9Cat commented Dec 28, 2024

Okay, I'll merge it to main and then take a look at the upstream change in #103.

PseudoAccel is definitely a WIP feature but complete for now. It compiles and runs. The CENTER acceleration looks quite reasonable for model that is not itself a satellite. The AXIS rotational acceleration might be a bit sensitive, but it needs to be tested with a large run to be sure. Both may need tuning and fancier smoothing for real applications; we will see. For nEJaccel: 0, the original behavior should obtain, so I think this addition should be safe enough for now.

@The9Cat The9Cat merged commit 9db72e7 into main Dec 28, 2024
8 checks passed
@The9Cat The9Cat deleted the minorFixesAndUpdates branch December 28, 2024 14:14
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