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

Add some methods to projective morphisms (rational maps) #33953

Closed
kwankyu opened this issue Jun 5, 2022 · 30 comments
Closed

Add some methods to projective morphisms (rational maps) #33953

kwankyu opened this issue Jun 5, 2022 · 30 comments

Comments

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

Add graph(), projective_degrees() and degree() methods to projective morphisms (rational maps).

Projective morphisms in Sage represent rational maps. That is, the domain of the definition of the morphism could be a dense subset of its domain.

Depends on #33950

Component: algebraic geometry

Author: Kwankyu Lee

Branch/Commit: ac60efe

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/33953

@kwankyu kwankyu added this to the sage-9.7 milestone Jun 5, 2022
@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 5, 2022

Author: Kwankyu Lee

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 5, 2022

Branch: u/klee/33953

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Commit: a39ca95

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

d2b1b06Add modules over domain
83cbb2bMerge branch 'develop' into module-over-domain
5e5b8f1Add submodule.py
74a3930Clean up doc tree of modules
c725f74Small fix on the main toc
c4e61dbAdd free resolutions
a39ca95Add projective_degrees() and degrees() methods

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 5, 2022

New commits:

d2b1b06Add modules over domain
83cbb2bMerge branch 'develop' into module-over-domain
5e5b8f1Add submodule.py
74a3930Clean up doc tree of modules
c725f74Small fix on the main toc
c4e61dbAdd free resolutions
a39ca95Add projective_degrees() and degrees() methods

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 5, 2022

Dependencies: #33950

@kwankyu kwankyu changed the title Add degree() method to projective morphisms (aka rational maps) Add some methods to projective morphisms (aka rational maps) Jun 5, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Changed commit from a39ca95 to 5c1e6e0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

5c1e6e0Fix a doctest; now gives the correct answer

@kwankyu

This comment has been minimized.

@kwankyu kwankyu changed the title Add some methods to projective morphisms (aka rational maps) Add some methods to projective morphisms (rational maps) Jun 5, 2022
@kwankyu

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2022

Changed commit from 5c1e6e0 to 4c3f8ce

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

4c3f8ceModify two doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2022

Changed commit from 4c3f8ce to 4c2e151

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b2870daRefactor FreeModule_generic
fcb51daAdd coercion map from a submodule to the free module
c4e89b9A fix to cooperate with matrix infrastructure
9f12e32Rename to Module_free_ambient and fix doctests
59226b1Add free resolutions
4c2e151Add some methods to projective morphisms

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e8db541Add some methods to projective morphisms

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2022

Changed commit from 4c2e151 to e8db541

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

0232e9eAdd free resolutions
bdbb479Merge branch 'develop' into t/33950/free-resolution
fcb1842Merge branch 't/33950/free-resolution' into t/33953/degree-morphism

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2022

Changed commit from e8db541 to fcb1842

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2022

Changed commit from fcb1842 to dabe765

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

817e341Merge branch 'develop' into t/33953/degree-morphism
736d448Moving the main computation to be done on demand.
5285e67Moving Singular conversion code to libs/singular; converting resoltuion files to Python files.
5917129Changing the doc for clarity and to add some more descriptions.
3f1d1c3Merge branch 'develop' into t/33950/public/rings/free_multigraded_resolutions-33950
ab725f2Fix spaces
dabe765Merge branch 't/33950/public/rings/free_multigraded_resolutions-33950' into t/33953/degree-morphism

@tscrim
Copy link
Collaborator

tscrim commented Aug 22, 2022

comment:14

There seems to be some real doctest failures reported by the patchbot:

sage -t --long --random-seed=28903988791302297014049808319140034392 src/sage/schemes/projective/projective_morphism.py  # 1 doctest failed

which I can reproduce. The others

sage -t --long --random-seed=28903988791302297014049808319140034392 src/sage/homology/graded_resolution.py  # 1 doctest failed
sage -t --long --random-seed=28903988791302297014049808319140034392 src/sage/homology/free_resolution.py  # 5 doctests failed

I cannot.

Here are my comments:

Since projective_degrees() potentially is an expensive computation, I would cache the result (which would also mean it returns a tuple).

Perhaps we should call morphism() instead as_morphim() to indicate that it is an equivalent but different type of object?

@tscrim
Copy link
Collaborator

tscrim commented Aug 22, 2022

comment:15

Also, if you could split this line:

-n1 = n + 1; m1 = m + 1
+n1 = n + 1
+m1 = m + 1

for our style guidelines (see patchbot).

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 22, 2022

comment:16

Replying to @tscrim:

There seems to be some real doctest failures reported by the patchbot:

sage -t --long --random-seed=28903988791302297014049808319140034392 src/sage/schemes/projective/projective_morphism.py  # 1 doctest failed

which I can reproduce.

Yes. It is related with a deep-rooted problem of conic curves modules. I am working on it. If I fail, then I would simply change the doctest slightly.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 22, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

3947c02Refactor conic curves
ac60efeFixes for reviewer comments

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 22, 2022

Changed commit from dabe765 to ac60efe

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 22, 2022

comment:18

Replying to @tscrim:

There seems to be some real doctest failures reported by the patchbot:

sage -t --long --random-seed=28903988791302297014049808319140034392 src/sage/schemes/projective/projective_morphism.py  # 1 doctest failed

which I can reproduce.

Fixed.

Since projective_degrees() potentially is an expensive computation, I would cache the result (which would also mean it returns a tuple).

Done. Also graph() is cached.

Perhaps we should call morphism() instead as_morphim() to indicate that it is an equivalent but different type of object?

Yes. Done.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 22, 2022

comment:19

Replying to @tscrim:

Also, if you could split this line:

-n1 = n + 1; m1 = m + 1
+n1 = n + 1
+m1 = m + 1

for our style guidelines (see patchbot).

Done. Thanks.

@tscrim
Copy link
Collaborator

tscrim commented Aug 22, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 22, 2022

comment:21

Thank you. Green bot => positive review.

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

Changed branch from u/klee/33953 to ac60efe

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

No branches or pull requests

3 participants