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

refactor(algebra/*): reduce use of old_structure_cmd in sub-objects #9774

Closed
wants to merge 15 commits into from

Conversation

semorrison
Copy link
Collaborator

@semorrison semorrison commented Oct 17, 2021

This removes the use of old_structure_cmd in sub(semi)ring, subalgebra, subfield, and intermediate_field.

Still to go is (lie_)submodule and sub_mul_action; not sure yet whether that will go in this PR or separately.


Open in Gitpod

@semorrison semorrison added WIP Work in progress awaiting-CI The author would like to see what CI has to say before doing more work. labels Oct 17, 2021
@@ -36,8 +41,23 @@ variables [comm_semiring R]
variables [semiring A] [algebra R A] [semiring B] [algebra R B] [semiring C] [algebra R C]
include R

/-- An alternative constructor for subalgebras which does not require separate proofs that
the subalgebra contains 0 and 1. -/
def mk' (s : set A)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def mk' (s : set A)
@[simp] def mk' (s : set A)

else you have to repeat all the coe_mk-type defs for coe_mk'

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 seems we don't have coe_mk-type defs for subalgebra in any case, and the lack of them (or of coe_mk' lemmas) is not currently hurting anything, so perhaps this is a separate issue to this PR?

let s : subalgebra R (tensor_algebra R M) := {
carrier := C,
mul_mem' := h_mul,
add_mem' := h_add,
algebra_map_mem' := h_grade0, },
let s : subalgebra R (tensor_algebra R M) := subalgebra.mk' C h_add h_mul h_grade0,
Copy link
Member

Choose a reason for hiding this comment

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

I'd argue this is less readable than it was before, and have mentioned in on Zulip. I'm not sure we can justify removing old_structure_cmd simply because it uses the word "old" ;).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See response on zulip. I think "simply because it uses the word "old"" doesn't quite get at it. :-)

@github-actions github-actions bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Oct 17, 2021
@github-actions github-actions bot removed awaiting-CI The author would like to see what CI has to say before doing more work. blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Oct 18, 2021
@github-actions
Copy link

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@semorrison semorrison added maybe-later and removed WIP Work in progress labels Oct 18, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 18, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 19, 2021
@semorrison
Copy link
Collaborator Author

I've cherrypicked out a few of the "stability" changes to downstream proofs, and made them as separate PRs, so I'll close this for now.

@semorrison semorrison closed this Oct 19, 2021
@YaelDillies YaelDillies deleted the subobject_old_structure_cmd branch August 13, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants