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

Work on issue #2015: grp_pow and related things #2071

Merged
merged 4 commits into from
Sep 8, 2024

Conversation

ndcroos
Copy link
Contributor

@ndcroos ndcroos commented Aug 31, 2024

Fixes #2015

(** [grp_pow] satisfies a multiplicative law of exponents. *)
Definition grp_pow_int_mul {G : Group} (m n : Int) (g : G)
: grp_pow g (m * n)%int = grp_pow (grp_pow g m) n.
(* This will follow from the previous two. *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(* This will follow from the previous two. *)

@@ -29,7 +29,7 @@ Definition rng_int_mult (R : Ring) := grp_pow_homo : R -> Int -> R.

(** [rng_int_mult R 1] preserves multiplication. This requires that the specified ring element is the unit. *)
Global Instance issemigrouppreserving_mult_rng_int_mult (R : Ring)
: IsSemiGroupPreserving (A:=cring_Z) (Aop:=(.*.)) (Bop:=(.*.)) (rng_int_mult R 1).
: IsSemiGroupPreserving (A:=cring_Z) (Aop:=(.*.)) (Bop:=(.*.)) (@rng_int_mult R 1).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
: IsSemiGroupPreserving (A:=cring_Z) (Aop:=(.*.)) (Bop:=(.*.)) (@rng_int_mult R 1).
: IsSemiGroupPreserving (A:=cring_Z) (Aop:=(.*.)) (Bop:=(.*.)) (rng_int_mult R 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot this change in this commit. I will do this tomorrow.

@jdchristensen
Copy link
Collaborator

It's probably also worth stating a third variant of grp_pow_neg, namely that grp_pow (- g) n = - grp_pow g n. This can be proved by combining grp_pow_neg and grp_pow_neg_inv, or maybe it's just as easy to prove it directlly.

Then ab_mul will follow from grp_pow_unit, grp_pow_mul and this new result, so ab_mul_helper won't be needed.

Comment on lines 71 to 72
- simpl.
unfold rng_int_mult.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation here seems off. Everything after a - should be two spaces indented so that you can easily see the parts of the proof.

- foo.
  bar.
- foo.
  bar.

@@ -85,3 +114,4 @@ Proof.
lhs_V nrapply (rng_homo_plus g).
f_ap.
Defined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 623 to 624
rewrite int_add_neg_l.
easy.
Copy link
Collaborator

Choose a reason for hiding this comment

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

easy is a bit weird, I would prefer if you instead used by or better yet, just explicitly use a tactic.

@ndcroos
Copy link
Contributor Author

ndcroos commented Sep 5, 2024

I haven't found a proof for issemigrouppreserving_mult_rng_int_mult that uses the suggested results (#2015 (comment)). (I did some attempts.)
I have a proof that for ab_mul that uses grp_pow_neg_inv' (another variant), but it does't usage of that result isn't crucially in the proof.
I also have an issue with grp_pow_neg_inv' that isn't found in the environment of AbelianGroup.v

@Alizter Alizter self-requested a review September 6, 2024 15:22
@jdchristensen
Copy link
Collaborator

@ndcroos There are a few problems with this PR:

  • The "Apply suggested changes" commit does more than just suggested changes, and the library doesn't build after that commit. You shouldn't push something that doesn't build unless you specifically are stuck on something and want help. You also shouldn't mix suggested changes with other work.
  • rng_int_mult_dist was simultaneously moved and changed, which means we can't see the changes in the diff. And the changes seem to make the proof harder to follow. I would move it back to where it was and undo the changes to the proof.
  • I think you should go back to before that commit, and make changes again, one at a time, checking that the library builds after each one. When you find the change that causes problems with universe variables, we can help fix it. But as it is now, too much is changed to make that easy to do.
  • The proof of ab_mul was replaced with a longer proof, so this is a step backwards. We'll need to discuss, but for now, I would revert to the previous proof.
  • The comment describing ab_mul was deleted and should be reinstated.

After things are in a good state again, it'll be easier to work on issemigrouppreserving_mult_rng_int_mult.

@ndcroos
Copy link
Contributor Author

ndcroos commented Sep 8, 2024

Thanks for the feedback. Could I get some hints for issemigrouppreserving_mult_rng_int_mult and ab_mul?

@jdchristensen
Copy link
Collaborator

The proof of issemigrouppreserving_mult_rng_int_mult is just the composite of the distributivity result and the power result. With the distributive law that I stated, you'd also need to use the commutativity of multiplication in Z, but if you also prove the reverse distributivity, it's just the composite of the two results. Since it's only two lines, I pushed it. And I gave a short proof of the distributivity laws. I'm out of time now, but will think about ab_mul later.

@jdchristensen
Copy link
Collaborator

And ab_mul is just a special case of grp_pow_mul. So I pushed that.

@jdchristensen jdchristensen changed the title Work on issue #2015 Work on issue #2015: grp_pow and related things Sep 8, 2024
@jdchristensen
Copy link
Collaborator

I tightened up a few proofs and think this is ready to merge. @Alizter , do you want to give it a quick check?

Before merging, I think we should squash most of the commits, since there was a lot of churn and some intermediate commits don't build. I'll do this after both @Alizter and @ndcroos respond (so @ndcroos can see the last few commits I made).

@jdchristensen
Copy link
Collaborator

@ndcroos A few general comments about how this ended up working out:

  • About ab_mul being a homomorphism, this is the statement that n (a + b) = n a + n b. In multiplicative notation, n a is a^n, so this goal translates to (a * b)^n = a^n * b^n, which is grp_pow_mul.
  • In general, instead of proving things about a specific situation (e.g. rng_int_mult or ab_mul) we tried to find a more basic result about grp_pow or int_iter that covered it.
  • We tried to avoid induction over n if we could instead apply a general principle, such as commuting with a group homomorphism.

Copy link
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ndcroos
Copy link
Contributor Author

ndcroos commented Sep 8, 2024

Looks also good to me.

@jdchristensen jdchristensen merged commit d2da27a into HoTT:master Sep 8, 2024
22 checks passed
@jdchristensen
Copy link
Collaborator

Thanks, @ndcroos!

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

Successfully merging this pull request may close these issues.

grp_pow and related things
3 participants