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.

Sign up for GitHub

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

Jump to bottom

Deprecate is_SchemeMorphism, is_SchemeTopologicalPoint #38296

Merged
merged 8 commits into from
Aug 10, 2024
Merged

Deprecate is_SchemeMorphism, is_SchemeTopologicalPoint #38296

merged 8 commits into from
Aug 10, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jun 27, 2024 •
edited
Loading

Part of:

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 28, 2024

The lint failures are unrelated and come from the release of ruff 0.5.0 (https://pypi.org/project/ruff/#history)

@mkoeppe mkoeppe changed the title Deprecate is_SchemeTopologicalPoint Deprecate is_SchemeTopologicalPoint Jun 28, 2024
@mkoeppe mkoeppe self-assigned this Jun 28, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 28, 2024 •
edited
Loading

Documentation preview for this PR (built with commit de805b1; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe mkoeppe changed the title Deprecate is_SchemeTopologicalPoint Deprecate is_SchemeMorphism, is_SchemeTopologicalPoint Jun 28, 2024
@mkoeppe mkoeppe marked this pull request as ready for review June 28, 2024 03:50
@tscrim
Copy link
Collaborator

tscrim commented Jul 1, 2024

Here is an interesting case of a place where it might be good to leave the is_SchemeMorphism (possibly renamed) as the second class to test doesn't fit the naming scheme (pun intended) one would expect. It's not so clear-cut to me to remove it. I'm not opposed, but just want to see if you have any second thoughts as well.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 1, 2024

Well, what could be useful here: a new ABC. But I definitely don't want to wait with the removal of is_Class functions until ABCs for all situations have been figured out.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 1, 2024

... or, alternatively, whether what is intended in the uses of is_SchemeMorphism could be replaced by using instance(..., Morphism) and ....category_for() .... .

@mkoeppe mkoeppe force-pushed the is_SchemeTopologicalPoint branch from 94f33e8 to 402e990 Compare July 10, 2024 01:48
@mkoeppe mkoeppe force-pushed the is_SchemeTopologicalPoint branch from 402e990 to 92a768a Compare July 17, 2024 08:14
@mkoeppe mkoeppe requested a review from kwankyu July 17, 2024 08:15
@kwankyu
Copy link
Collaborator

kwankyu commented Jul 17, 2024 •
edited
Loading 

sage: from sage.schemes.elliptic_curves.ell_point import EllipticCurvePoint_field
sage: from sage.schemes.generic.morphism import SchemeMorphism
sage: issubclass(EllipticCurvePoint_field, SchemeMorphism)
True

This is also mathematically sound.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 17, 2024

Thanks. So I can actually simplify many isinstance calls.

kwankyu
kwankyu approved these changes Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 17, 2024

Thank you!

@vbraun
Copy link
Member

vbraun commented Jul 24, 2024

Merge conflict

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 5, 2024
sagemathgh-38296: Deprecate is_SchemeMorphism, `is_SchemeTopologica…
f0f8a09 
…lPoint`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Part of:
- sagemath#32414

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38296
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit 5be4da7 into sagemath:develop Aug 10, 2024
17 of 19 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 10, 2024
@mkoeppe mkoeppe deleted the is_SchemeTopologicalPoint branch August 10, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwankyu kwankyu kwankyu approved these changes

Assignees

@mkoeppe mkoeppe

Labels
c: algebraic geometry t: refactoring v: moderate
Projects
None yet
Milestone
sage-10.5
Development

Successfully merging this pull request may close these issues.
4 participants
@mkoeppe @tscrim @kwankyu @vbraun