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(*): bundle is_field_hom #717

Closed
wants to merge 2 commits into from
Closed

refactor(*): bundle is_field_hom #717

wants to merge 2 commits into from

Conversation

kckennylau
Copy link
Collaborator

TO CONTRIBUTORS:

Make sure you have:

  • reviewed and applied the coding style: coding, naming
  • for tactics:
  • make sure definitions and lemmas are put in the right files
  • make sure definitions and lemmas are not redundant

If this PR is related to a discussion on Zulip, please include a link in the discussion.

For reviewers: code review check list

@@ -96,13 +96,19 @@ noncomputable instance field : discrete_field (adjoin_root f) :=
..adjoin_root.comm_ring f,
..ideal.quotient.field (span {f} : ideal (polynomial α)) }

instance : is_field_hom (coe : α → adjoin_root f) := by apply_instance
variables (α f)
def fof : α →f adjoin_root f :=
Copy link
Member

Choose a reason for hiding this comment

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

I understand the f-prefix. Yet, intuitively I would just call this of.


instance lift_is_field_hom [field β] {i : α → β} [is_ring_hom i] {a : β}
{h : f.eval₂ i a = 0} : is_field_hom (lift i a h) := by apply_instance
def flift [field β] (i : α → β) [is_ring_hom i] (a : β)
Copy link
Member

Choose a reason for hiding this comment

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

And this lift.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's already lift.

@ChrisHughes24
Copy link
Member

No point making field hom and ring hom separate, especially if they're bundled. field hom should probably be reducibly defined to be ring hom.

@kckennylau
Copy link
Collaborator Author

I agree, but I intend to test bundling on field homs only. If the consensus approves for bundling of ring homs, then field hom would be defined to be ring hom (or even semiring hom).

@jcommelin
Copy link
Member

So, what do we do with this experiment? @digama0 seemed to be in favour of bundling. What do others think? Do we need extended discussion? @ChrisHughes24 @kckennylau @rwbarton @avigad @semorrison
If this experiment is decided to be a succes, but we don't want to merge it because in fact we should really bundle all algebraic morphisms, then we should start a big refactor project.

@ChrisHughes24
Copy link
Member

My main concern with this is that we end up having to do type class inference by hand, if we want a field hom to be a group hom or whatever. I'm not sure how much of a problem this is going to be. The main upside is that the simp attributes work. I'd love to get unification hints to solve this problem, but I couldn't get it to work.

@semorrison
Copy link
Collaborator

semorrison commented Feb 27, 2019 via email

@jcommelin
Copy link
Member

Should we close this PR? There is no intention to merge this, because it is mostly an experiment. Scott indicates that there should be more experimenting before we know is we want bundled homs in the algebraic hierarchy. I don't think anyone is actively working on the experiment currently. We can always reopen this PR if someone returns to this experiment.

@jcommelin
Copy link
Member

I'm going to close this one for now. We can reopen once we get back to the idea of bundling morphisms.

@jcommelin jcommelin closed this May 6, 2019
@kbuzzard kbuzzard mentioned this pull request May 17, 2019
@ChrisHughes24 ChrisHughes24 deleted the field-hom branch June 26, 2019 13:11
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.

None yet

4 participants