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

Mark as obsolete: ZeroSM/ZeroAttr, AdditiveInverseSM/AdditiveInverseAttr OneSM/OneAttr, InverseSM/InverseAttr, TransposedMatAttr #5263

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

fingolfin
Copy link
Member

Currently we have these three variants of One, together with a bunch of synonyms for each:

  • OneImmutable == One == OneAttr == Identity
  • OneMutable == OneOp
  • OneSameMutability == OneSM

This is then repeated for Zero, Inverse, AdditiveInverse.

The delegation between these is already complicated enough, but each time I have to look into this, my head starts spinning due to the many synonyms, and I can't quite remember which is which. I propose we narrow this down a bit:

  • the variants with SM seems to be purely to avoid some typing, so I've removed all uses of them. Turns out no packages uses it, either. So we can mark them as obsolete.

  • the Attr variants are also almost unused, except for a single use in the FinInG package (removed in FinInG 1.5.4). Still, we can safely mark it as obsolete, and remove all users of it beyond the documentation. (As a bonus, also make TransposedMatAttr obsolete.)

  • the Op synonym to me also seems redundant. Alas, a ton of packages use them, so I mostly left those alone.

  • I think it is fine to keep One (and Identity) as synonyms for OneImmutable; at least for me personally it seems I can deal with that one synonym sufficiently well in my head.

Please provide a short summary of this PR and its purpose here. E.g., does it add a new feature, and which? Does it fix a bug, and which? If there is an associated issue, please list it here.

Text for release notes

We track noteworthy changes as entries in the CHANGES.md file in the root directory of this repository. To help us decide whether and how to describe your PR, please do one of the following:

  1. If this PR shall not be mentioned in the release notes, write "none" here.
  2. If a brief note suffices, edit the title of this PR to be usable as entry in CHANGES.md, and write "see title" here (or if you have the required permissions, add the label release notes: use title to this PR and remove this section)
  3. If a longer note is needed, just write it here, ideally following the general style used in that file

Further details

If necessary, provide further details down here.

@fingolfin fingolfin added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Dec 13, 2022
@fingolfin fingolfin mentioned this pull request Dec 13, 2022
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

This looks good.
(Apparently there is still one instance of AdditiveInverseAttr in tst/testspecial/bad-minus.g.out.)

Concerning the history of the names in question, I think that Zero (for the mathematical concept) is the oldest name, and ZeroAttr and ZeroOp had been introduced at the same time in order to specify an immutable and a mutable zero.
If I remember well then the names ZeroImmutable and ZeroMutable have been introduced later.
Names of the form SomethingAttr and SomethingOp occur also in other situations (for example BlocksAttr and BlocksOp), but they are usually undocumented.

Currently we have these three variants of `One`, together
with a bunch of synonyms for each:

- `OneImmutable` == `One` == `OneAttr` == `Identity`
- `OneMutable` == `OneOp`
- `OneSameMutability` == `OneSM`

This is then repeated for `Zero`, `Inverse`, `AdditiveInverse`.

The delegation between these is already complicated enough, but each
time I have to look into this, my head starts spinning due to the many
synonyms, and I can't quite remember which is which. I propose we
narrow this down a bit:

- the variants with `SM` seems to be purely to avoid some typing, so
  I've removed all uses of them. Turns out no packages uses it, either.
  So we can mark them as obsolete.

- the `Attr` variants are also almost unused, except for a single use in
  the FinInG package (removed in FinInG 1.5.4). Still, we can safely
  mark it as obsolete, and remove all users of it beyond the
  documentation. (As a bonus, also make `TransposedMatAttr` obsolete.)

- the `Op` synonym to me also seems redundant. Alas, a ton of packages
  use them, so I mostly left those alone.

- I think it is fine to keep `One` (and `Identity`) as synonyms for
  `OneImmutable`; at least for me personally it seems I can deal with
  that one synonym sufficiently well in my head.
@fingolfin
Copy link
Member Author

@ThomasBreuer thanks for the explanation regarding the background, that's indeed illuminating, and makes a lot of sense.

@fingolfin fingolfin merged commit 150c227 into gap-system:master Dec 14, 2022
@fingolfin fingolfin deleted the mh/SM branch December 14, 2022 12:39
@fingolfin fingolfin added the kind: removal or deprecation A feature was removed or deprecated / made obsolete label Jan 28, 2024
@fingolfin fingolfin changed the title Mark as obsolete: ZeroSM/ZeroAttr, AdditiveInverseSM/AdditiveInverseAttr OneSM/OneAttr, InverseSM/InverseAttr, TransposedMatAttr Mark as obsolete: ZeroSM/ZeroAttr, AdditiveInverseSM/AdditiveInverseAttr OneSM/OneAttr, InverseSM/InverseAttr, TransposedMatAttr Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: removal or deprecation A feature was removed or deprecated / made obsolete release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants