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

Update localizations and schemes. #1110

Merged
merged 12 commits into from
Mar 9, 2022

Conversation

HechtiDerLachs
Copy link
Collaborator

Some updates and extensions regarding the localizations and schemes.

This makes use of @thofma 's new code on polynomial maps.

Copy link
Collaborator

@thofma thofma left a comment

Choose a reason for hiding this comment

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

Minor comment on a name. Since we have isisomorphic, I would suggest to change canonically_isomorphic to is[_]canonically_isomorphic.

@HechtiDerLachs
Copy link
Collaborator Author

It seems that there is a conflict with the plane curves about the definition of ProjectiveScheme. These files are still in the experimental folder.

  • Who is using them?
  • Is anyone still developing this? Who can I talk to about making the two patches coherent?
  • If there's nobody available for the latter: How important is it that the projective spaces from the plane curves are being kept?
    CC: @wdecker

@HechtiDerLachs
Copy link
Collaborator Author

This PR is already quite big again, but it can be split in two main patches:

  • the update on existing code in Oscar up to this commit
  • some more extensions on glueings and projective schemes.

If desired, we could therefore split it into two PRs. Let me know if this would be better.

@thofma
Copy link
Collaborator

thofma commented Feb 23, 2022

I can't say much about the scheme stuff. From the map point of view of this looks good. Did you see my comment about is[_]canonically_isomorphic?

@HechtiDerLachs
Copy link
Collaborator Author

Ok, thanks for having a look at it!

Yes, I saw your comment and I'm probably going to change it. We were not very happy with the name, anyway, so if there are any suggestions for something shorter, let me know.

BTW: I'm just running some performance tests and the hom in MPolyRing.jl, line 25, seems to be a bottleneck. Any ideas why?

@thofma
Copy link
Collaborator

thofma commented Feb 28, 2022

Do you want to rename or should I just merge it as is?

@HechtiDerLachs
Copy link
Collaborator Author

Sorry, but I'm afraid, I should probably have another look at this PR. At some points (outside the MPoly(Quo)LocalizedRingHoms) I'm still using the old AlgebraHomomorphism. Should I clean this up before merging?

@thofma
Copy link
Collaborator

thofma commented Mar 4, 2022

That would be great

@HechtiDerLachs
Copy link
Collaborator Author

I will rename the canonically_isomorphic routine. I also need to insert some type assertions. Sorry for not managing to do this today.

But the failing tests are not my fault, are they?

@thofma
Copy link
Collaborator

thofma commented Mar 5, 2022

Yes, you can ignore them.

@HechtiDerLachs
Copy link
Collaborator Author

Just a heads up: This should be ready to be merged now. The failing checks are for other reasons.

@thofma thofma merged commit b02ea95 into oscar-system:master Mar 9, 2022
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

2 participants