Skip to content

Remove all Cephes source files except those needed for igami#2371

Merged
dellaert merged 3 commits intoborglab:developfrom
Gold856:trim-down-cephes
Jan 31, 2026
Merged

Remove all Cephes source files except those needed for igami#2371
dellaert merged 3 commits intoborglab:developfrom
Gold856:trim-down-cephes

Conversation

@Gold856
Copy link
Copy Markdown
Contributor

@Gold856 Gold856 commented Jan 24, 2026

Addresses #2337.

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Jan 24, 2026

I just noticed that Eigen has reimplementations of some cephes functions as well, including igammac. I wonder if that's sufficient to reimplement igami or if we should continue to delete cephes code.

@dellaert
Copy link
Copy Markdown
Member

If that can be done, and we can kill cephes, that would definitely be preferable.
PS chi_squared_quantile is also not named correctly: should be camelCase.

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Jan 25, 2026

Renamed chi_squared_quantile. I'm not a math person, so no idea where to even start with reimplementing igami. We'll stick with our shrunken copy of cephes for now.

@talregev
Copy link
Copy Markdown
Contributor

I'm not a math person, so no idea where to even start with reimplementing igami.

AI can help you. you can try next PR.

@ProfFan
Copy link
Copy Markdown
Collaborator

ProfFan commented Jan 26, 2026

I don't think there is a need for reimplementation. @Gold856 could you rename all functions by adding a gtsam_cephes_ prefix to all of them in these files that still not removed? This way we won't have symbol clashes.

Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Thanks! I’d still be happier if we could remove the entire third-party cephes folder, for example by creating a single c file inside that internal directory. But this is an improvement, so all for it.

One request: I think maybe we should probably add some text in cephes README (or add one) saying what we did here, and mention it’s done in accordance with license?

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Jan 30, 2026

Done and done.

@dellaert
Copy link
Copy Markdown
Member

Oopsies, compilation issue

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Jan 31, 2026

Whoops! I forgot to rename the igami call in gtsam itself (even though I already had the thought in my head at one point...)

Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!!

@dellaert dellaert merged commit 9f995fb into borglab:develop Jan 31, 2026
33 of 34 checks passed
@Gold856 Gold856 deleted the trim-down-cephes branch January 31, 2026 03:23
@varunagrawal
Copy link
Copy Markdown
Contributor

I don't think there is a need for reimplementation. @Gold856 could you rename all functions by adding a gtsam_cephes_ prefix to all of them in these files that still not removed? This way we won't have symbol clashes.

Sorry for the late comment, but this is problematic since it prevents us from easily updating cephes in the future (as long as we need it).
It's interesting that cephes is causing these many issues when Boost didn't have this problem. Is it because Boost did namespacing better?

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Feb 1, 2026

It doesn't look like cephes is really getting any updates... I doubt that will be an issue. Easy to revert things back though, but yeah, I kinda dislike renaming symbols because it feels a bit icky to vendor a library then modify it with the only tracking mechanism being git.

I would be surprised if Boost had reimplementations of C math functions. It wouldn't be out of place for Cephes, being from the literal 80s before even ANSI C was a thing, to contain C math functions for platforms/compilers that didn't have implementations. Boost on the other hand would've been able to rely on <cmath>. Unless we're talking about symbol conflicts with other copies of the same library, e.g. a Cephes copy from another library. It wasn't clear what "symbol clashes" meant.

@dellaert
Copy link
Copy Markdown
Member

dellaert commented Feb 1, 2026

I don't like to talk about cephes updates :-). Boost is not an option. We're trying to purge it.

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