Skip to content

refactor(GroupTheory/Submonoid/Operations): Use SubmonoidClass and delete redundant instances#11304

Closed
tb65536 wants to merge 14 commits intomasterfrom
tb_monoid_action
Closed

refactor(GroupTheory/Submonoid/Operations): Use SubmonoidClass and delete redundant instances#11304
tb65536 wants to merge 14 commits intomasterfrom
tb_monoid_action

Conversation

@tb65536
Copy link
Copy Markdown
Contributor

@tb65536 tb65536 commented Mar 11, 2024

This PR refactors some SMul instances in GroupTheory/Submonoid/Operations.lean to use SubmonoidClass. This makes many other similar instances redundant.


Open in Gitpod

@tb65536 tb65536 added awaiting-review awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. t-algebra Algebra (groups, rings, fields, etc) labels Mar 11, 2024
@github-actions github-actions bot removed the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label Mar 12, 2024
@jcommelin
Copy link
Copy Markdown
Member

This seems like a good move. Let's also bench it.

!bench

@jcommelin
Copy link
Copy Markdown
Member

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit b3d2126.
There were significant changes against commit 68f3af6:

  Benchmark                     Metric         Change
  ===================================================
- build                         wall-clock      22.2%
- ~Mathlib.Algebra.Module.PID   instructions     5.7%

@eric-wieser
Copy link
Copy Markdown
Member

Maybe not so good an idea!

@jcommelin
Copy link
Copy Markdown
Member

@tb65536 Given this bench result, what do you think? Do you want to attempt to improve performance, or shall we punt on this?

@jcommelin jcommelin added awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-review labels Mar 26, 2024
@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 28, 2024
@tb65536
Copy link
Copy Markdown
Contributor Author

tb65536 commented Mar 28, 2024

Probably just punt for now.

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

Labels

awaiting-author A reviewer has asked the author a question or requested changes. merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants