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

Add interface for optionally static size #160

Merged
merged 8 commits into from
Aug 3, 2023
Merged

Conversation

mateuszbaran
Copy link
Member

Basic infrastructure for solving JuliaManifolds/Manifolds.jl#625 .

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #160 (c35a1be) into master (71f85b6) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head c35a1be differs from pull request most recent head 9d68f5b. Consider uploading reports for the commit 9d68f5b to get more accurate results

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   99.96%   99.84%   -0.12%     
==========================================
  Files          19       20       +1     
  Lines        2539     2553      +14     
==========================================
+ Hits         2538     2549      +11     
- Misses          1        4       +3     
Files Changed Coverage Δ
src/DefaultManifold.jl 100.00% <100.00%> (ø)
src/maintypes.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

Nice that you started this.

While I think this is quite technical and should not concern beginners, it might be nice for advanced usages.
A small benchmark illustrating when which is nicer might be nice as well (if that can be well-illustrated)

src/maintypes.jl Outdated Show resolved Hide resolved
src/DefaultManifold.jl Outdated Show resolved Hide resolved
src/maintypes.jl Outdated Show resolved Hide resolved
@mateuszbaran
Copy link
Member Author

While I think this is quite technical and should not concern beginners, it might be nice for advanced usages.
A small benchmark illustrating when which is nicer might be nice as well (if that can be well-illustrated)

Yes, it's definitely not for beginners.
Benchmarks would be hard at the moment because all good examples live in Manifolds.jl. I think rotation matrices would make a great example for a benchmark.

@kellertuer
Copy link
Member

Sure let's just keep that a bit in mind to explain this feature with a benchmark somewhen.
I think I see advantages of both and disadvantages, but having a small tutorial on that would be great – sure probably only in Manifolds.jl

@mateuszbaran
Copy link
Member Author

Sure, I will add a clear explanation with an example in the Manifolds.jl PR that converts key manifolds to AbstractSize.

@mateuszbaran mateuszbaran added the preview docs Add this label if you want to see a PR-preview of the documentation label Jul 4, 2023
src/DefaultManifold.jl Outdated Show resolved Hide resolved
@mateuszbaran mateuszbaran merged commit c197b7c into master Aug 3, 2023
@kellertuer kellertuer deleted the mbaran/abstract-size branch May 4, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants