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

Rename Symplectic to SymplecticMatrices? #701

Closed
2 of 3 tasks
Tracked by #438 ...
kellertuer opened this issue Jan 3, 2024 · 5 comments · Fixed by #732
Closed
2 of 3 tasks
Tracked by #438 ...

Rename Symplectic to SymplecticMatrices? #701

kellertuer opened this issue Jan 3, 2024 · 5 comments · Fixed by #732

Comments

@kellertuer
Copy link
Member

kellertuer commented Jan 3, 2024

Currently we usually suffix Matrices for quite a few manifolds where the name itself is not specific enough, e.g. SymmetricMatrices since we use Symmetric for the single matrix.

This is a bit inconsistent to one other case we have, which I therefore would like to rename

  • Symplectic should be called SymplecticMatrices
  • SynplecticMatrix should be called SymplecticElement
  • Symplectic could be introduced as a type indicating a symplectic matrix (similar to Symmetric)

I think this would even only be mildly breaking, only the constructors need to be deprecated but could still keep working for a while. Breaking would be if other use these types, since we could not const them when reusing them. But it would be nice to have this consistently with LinearAlgebra in the naming

What are opinions on this? I am reworking a bit of this in #700, so I could even include it in there is the breaking part is considered fine enough.

@kellertuer kellertuer changed the title Rename Symplectic to Symplecticmatrices Rename Symplectic to SymplecticMatrices? Jan 3, 2024
@mateuszbaran
Copy link
Member

I think this renaming makes sense. We can schedule it for the next breaking release.

@kellertuer
Copy link
Member Author

Cool. I will carefully add the functionality I need on #700, and will maybe already add a few notes what might change in the future as comments

@kellertuer kellertuer added this to the v0.10.0 milestone Jan 3, 2024
@mateuszbaran mateuszbaran mentioned this issue Jan 3, 2024
12 tasks
@kellertuer
Copy link
Member Author

If we just deprecate the constructors, the only breaking part would be if someone dispatches on the old Symplectic type, the others could be kept as deprecated const definitions even.
Would that still be too bad to be considered breaking? I could otherwise include this in #700.

@mateuszbaran
Copy link
Member

I think we can do the first two points in #700 with deprecation and aliases but the third one IMO needs to wait for a breaking release.

@kellertuer
Copy link
Member Author

That sounds fair enough. I will do those two then next in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants