Skip to content

Conversation

The9Cat
Copy link
Member

@The9Cat The9Cat commented May 5, 2025

This is a WIP for ongoing development in the devel branch. This allows CI to run at the very least.

@The9Cat The9Cat marked this pull request as draft May 21, 2025 15:58
@The9Cat The9Cat changed the title Bump version number to 7.9.0 and merge recent bug fixes from main Bump version number to 7.9.0 and merge recent bug fixes from main [WIP] May 21, 2025
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.

Great feature additions! I've run a small suite of local tests, nothing unexpected. Just a couple flagged things to decide on before bumping the version, but we're ready for this sooner rather than later.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to actually remove this workaround in 7.9?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I'd split the question into two questions:

  1. When should we ditch all of the backward compatibility (if ever)?
  2. What should be the default behavior on a cache version change generally?

On the first: it might be too soon to remove old (pre-HDF5) cache support, but this should be done at some point. More generally, there is so much code here to support and I already feel like I have knowledge atrophy. So I'm in favor of beginning to trim out old stuff, when possible. The allow_old_cache is definitely a temporary kludge to work around the Eigen bug. I will argue that I should bumped the version number when I changed the code for the new Eigen API, although to be fair to myself, I had no idea that there would be a bug. Then, we would not have needed allow_old_cache. I will tentatively propose that we bump the version number now, add the appropriate version checking logic. I suppose we can suffer with allow_old_cache a bit longer.

On the second, there are two obvious options:

  1. Trigger a rebuild, which is the current workaround
  2. Throw an exception to force the user to delete the old cache
    I go both ways on this. On the one hand, it is a cache so overwriting seems logical. On the other hand, if the cache has historical value, the user should be forced to decide, which favors option 2.

}


std::vector<double> CBDisk::getAccel(double x, double y, double z)
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering: do we want to add CBDisk in the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can go either way on that. FlatDisk is a superset through its helper class EmpCyl2d which provides the CB basis. So one could get back CB by providing the CB density as the empirical target. Keeping CBDisk means another thing to maintain. But CBDisk is simple, easier to use, and good sanity check for a two-d expansion generally, and makes contact with a well-known example from the literature. So see a few more ticks in the keep than eliminate column. It's close.

"rmin",
"rmax",
"self_consistent",
"FIX_L0",
Copy link
Member

Choose a reason for hiding this comment

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

It occurs to me to flag this up for addition to docs.

Copy link
Member Author

@The9Cat The9Cat Jul 22, 2025

Choose a reason for hiding this comment

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

Yes!

And this raises another general issue: we now have a compendium of parameter keys for most important exp classes. We probably should spend a few days trying to document each and every key.

As of now, we are automatically importing our C++ documentation with Doxygen/CMake/Sphinx/Breathe linkage. This seems to be the current practice (after some quick googling) for most developers. Pretty ugly, tbh.

Right now, I'm hand importing the doxygen-generated xml into the sphinx tree. I suspect that we could automate this by putting the sphinx documentation into the main EXP tree and configuring readthedocs to access a subdirectory which was populated by CMake with the xml. I'm sure I could figure out how to do all of this, but it would be great to lean on someone that already has the expertise...

Martin D. Weinberg and others added 24 commits July 22, 2025 16:15
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>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
michael-petersen and others added 26 commits August 16, 2025 09:49
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>
Sure.  I agree that pre-increment better represents the intent.  Although post-increment is not wrong either.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Handle Spherical SL solutions for large harmonic orders
Add a factory member to BiorthBasis
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Two minor fixes necessary for successful compilation with g++15
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Implement rotation matrices for frame transformation
@michael-petersen michael-petersen marked this pull request as ready for review September 30, 2025 10:32
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.

We've been working with the bits and pieces in here for a while now, I think it makes sense to bring this functionality to main. Are there any other tests we would like to run?

Martin D. Weinberg and others added 2 commits September 30, 2025 14:10
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