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

[Merged by Bors] - refactor(*): rename semimodule to module, delete typeclasses module and vector_space #7322

Closed
wants to merge 22 commits into from

Conversation

Vierkantor
Copy link
Collaborator

@Vierkantor Vierkantor commented Apr 22, 2021

Splitting typeclasses between semimodule, module and vector_space was causing many (small) issues, so why don't we just get rid of this duplication?

This PR contains the following changes:

  • delete the old module and vector_space synonyms for semimodule
  • find and replace all instances of semimodule and vector_space to module
  • (thereby renaming the previous semimodule typeclass to module)
  • rename vector_space.dim to module.rank (in preparation for generalizing this definition to all modules)
  • delete a couple module instances that have now become redundant

This find-and-replace has been done extremely naïvely, but it seems there were almost no name clashes and no "clbuttic mistakes". I have gone through the full set of changes, finding nothing weird, and I'm fixing any build issues as they come up (I expect less than 10 declarations will cause conflicts).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/module.20from.20semimodule.20over.20a.20ring

A good example of a workaround required by the module abbreviation is the triv_sq_zero_ext.module instance.


Open in Gitpod

@Vierkantor Vierkantor added WIP Work in progress RFC Request for comment labels Apr 22, 2021
test/free_algebra.lean Outdated Show resolved Hide resolved
vector space: 'vector_space'
product of vector spaces: 'prod.semimodule'
vector space: 'module'
product of vector spaces: 'prod.module'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that an unusual name? module.prod would be more standard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the module instance on the prod type, therefore prod.module.

@Vierkantor Vierkantor removed the RFC Request for comment label Apr 23, 2021
@Vierkantor
Copy link
Collaborator Author

Marking this as ready to review since it builds and the reaction on Zulip was positive.

Copy link
Collaborator

@sgouezel sgouezel 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 to me, thanks a lot!
There is only one point about which I am unsure. After your refactor, we have both module.rank and findim, which don't go well together. Would finrank make sense? (And also adding somewhere a docstring saying that dimension is called rank, that would have enough keywords in it to be easy to find by grepping?)

src/algebra/module/ordered.lean Outdated Show resolved Hide resolved
src/algebra/module/ordered.lean Outdated Show resolved Hide resolved
src/algebra/module/projective.lean Outdated Show resolved Hide resolved
src/algebra/module/projective.lean Outdated Show resolved Hide resolved
src/analysis/normed_space/basic.lean Outdated Show resolved Hide resolved
src/analysis/normed_space/basic.lean Outdated Show resolved Hide resolved
src/linear_algebra/dimension.lean Outdated Show resolved Hide resolved
Co-authored-by: sgouezel <sebastien.gouezel@univ-rennes1.fr>
@Vierkantor
Copy link
Collaborator Author

After your refactor, we have both module.rank and findim, which don't go well together. Would finrank make sense?

I guess I might as well go ahead and change that. Can the finite_dimensional typeclass stay as it is, or also become finite_rank? (Does it hold for all modules that they have finite rank iff they are Noetherian?)

(And also adding somewhere a docstring saying that dimension is called rank, that would have enough keywords in it to be easy to find by grepping?)

In the docstring for module.rank I wrote: "In a vector space, this is the same as the dimension of the space." Do you have suggestions for something to add?

Vierkantor and others added 2 commits April 23, 2021 15:32
We still call keep calling `finite_dimensional` the same thing,
but we might decide to change that later.
Co-authored-by: sgouezel <sebastien.gouezel@univ-rennes1.fr>
@sgouezel
Copy link
Collaborator

Looks like the linter is not happy. Otherwise, let's get this merged quickly to avoid incoming conflicts.
bors d+

@bors
Copy link

bors bot commented Apr 23, 2021

✌️ Vierkantor can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Apr 23, 2021
@sgouezel
Copy link
Collaborator

bors r+

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Apr 24, 2021
bors bot pushed a commit that referenced this pull request Apr 24, 2021
…ule` and `vector_space` (#7322)

Splitting typeclasses between `semimodule`, `module` and `vector_space` was causing many (small) issues, so why don't we just get rid of this duplication?

This PR contains the following changes:
 * delete the old `module` and `vector_space` synonyms for `semimodule`
 * find and replace all instances of `semimodule` and `vector_space` to `module`
 * (thereby renaming the previous `semimodule` typeclass to `module`)
 * rename `vector_space.dim` to `module.rank` (in preparation for generalizing this definition to all modules)
 * delete a couple `module` instances that have now become redundant

This find-and-replace has been done extremely naïvely, but it seems there were almost no name clashes and no "clbuttic mistakes". I have gone through the full set of changes, finding nothing weird, and I'm fixing any build issues as they come up (I expect less than 10 declarations will cause conflicts).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/module.20from.20semimodule.20over.20a.20ring

A good example of a workaround required by the `module` abbreviation is the [`triv_sq_zero_ext.module` instance](https://github.com/leanprover-community/mathlib/blob/3b8cfdc905c663f3d99acac325c7dff1a0ce744c/src/algebra/triv_sq_zero_ext.lean#L164).



Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@bors
Copy link

bors bot commented Apr 24, 2021

Build failed:

@bryangingechen bryangingechen removed the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Apr 24, 2021
@sgouezel
Copy link
Collaborator

bors r+ p=37

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Apr 24, 2021
bors bot pushed a commit that referenced this pull request Apr 24, 2021
…ule` and `vector_space` (#7322)

Splitting typeclasses between `semimodule`, `module` and `vector_space` was causing many (small) issues, so why don't we just get rid of this duplication?

This PR contains the following changes:
 * delete the old `module` and `vector_space` synonyms for `semimodule`
 * find and replace all instances of `semimodule` and `vector_space` to `module`
 * (thereby renaming the previous `semimodule` typeclass to `module`)
 * rename `vector_space.dim` to `module.rank` (in preparation for generalizing this definition to all modules)
 * delete a couple `module` instances that have now become redundant

This find-and-replace has been done extremely naïvely, but it seems there were almost no name clashes and no "clbuttic mistakes". I have gone through the full set of changes, finding nothing weird, and I'm fixing any build issues as they come up (I expect less than 10 declarations will cause conflicts).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/module.20from.20semimodule.20over.20a.20ring

A good example of a workaround required by the `module` abbreviation is the [`triv_sq_zero_ext.module` instance](https://github.com/leanprover-community/mathlib/blob/3b8cfdc905c663f3d99acac325c7dff1a0ce744c/src/algebra/triv_sq_zero_ext.lean#L164).



Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: sgouezel <sebastien.gouezel@univ-rennes1.fr>
@bors
Copy link

bors bot commented Apr 24, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(*): rename semimodule to module, delete typeclasses module and vector_space [Merged by Bors] - refactor(*): rename semimodule to module, delete typeclasses module and vector_space Apr 24, 2021
@bors bors bot closed this Apr 24, 2021
@bors bors bot deleted the semimodule_to_module branch April 24, 2021 19:36
bors bot pushed a commit that referenced this pull request Apr 5, 2023
… of `dim` (#18735)

The `dim` name is left from the previous name of the function, `vector_space.dim`. When that was merged with `module.rank` in #7322, we left renaming the existing lemmas as future work.

This commit was made by replacing `(\b|(?<=_))dim(\b|(?=_))` with `rank` in the `dimension` and `finite_dimensional` files, and then manually fixing downstream breakages; it's important not to rename `power_basis.dim` at the same time!

Deciding whether to move some of these to the `module` namespace is left as future work, the diff is already big enough.
bors bot pushed a commit that referenced this pull request Apr 5, 2023
… of `dim` (#18741)

The `dim` name is left from the previous name of the function, `vector_space.dim`. When that was merged with `module.rank` in #7322, we left renaming the existing lemmas as future work.

This commit was made by replacing `(\b|(?<=_))dim(\b|(?=_))` with `rank` in the `dimension` and `finite_dimensional` files, and then manually fixing downstream breakages; it's important not to rename `power_basis.dim` at the same time!

Deciding whether to move some of these to the `module` namespace is left as future work, the diff is already big enough.
xroblot pushed a commit that referenced this pull request Apr 23, 2023
… of `dim` (#18741)

The `dim` name is left from the previous name of the function, `vector_space.dim`. When that was merged with `module.rank` in #7322, we left renaming the existing lemmas as future work.

This commit was made by replacing `(\b|(?<=_))dim(\b|(?=_))` with `rank` in the `dimension` and `finite_dimensional` files, and then manually fixing downstream breakages; it's important not to rename `power_basis.dim` at the same time!

Deciding whether to move some of these to the `module` namespace is left as future work, the diff is already big enough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants