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

implement height functions for product points #25821

Closed
raghukul01 opened this issue Jul 11, 2018 · 10 comments
Closed

implement height functions for product points #25821

raghukul01 opened this issue Jul 11, 2018 · 10 comments

Comments

@raghukul01
Copy link

Currently local_height() and global_height() function are not present in the product projective points.

CC: @bhutz @raghukul01

Component: algebraic geometry

Keywords: gsoc2018

Author: Raghukul Raman

Branch/Commit: af73f95

Reviewer: Ben Hutz

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

@raghukul01
Copy link
Author

@raghukul01
Copy link
Author

Commit: 44a8782

@raghukul01
Copy link
Author

Author: Raghukul Raman

@raghukul01
Copy link
Author

New commits:

44a878225821: Added global_height and local height for product points

@bhutz
Copy link

bhutz commented Jul 12, 2018

comment:3

Concept is fine, but the code can be made more readable

return max([self[i].global_height(prec=prec) \
                     for i in range(self.codomain().ambient_space().num_components())])

to

return max([t.global_height(prec=prec) for t in self])

I think more explanation is warranted in the docs as well. i.e., that you are taking the max of the heights of the component points.

and similarly for local height.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2018

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

af73f9525821: Updated docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2018

Changed commit from 44a8782 to af73f95

@raghukul01
Copy link
Author

comment:5

I think return max([t.global_height(prec=prec) for t in self] won't work since __iter__() function iterates over all coordinates, but here we need to iterate over all components. If we take max over all coordinates then we need to add a function to convert the space defined over QQbar to NumberField, as done in projective_point.

I have made it more readable though.

Replying to @bhutz:

Concept is fine, but the code can be made more readable

return max([self[i].global_height(prec=prec) \
                     for i in range(self.codomain().ambient_space().num_components())])

to

return max([t.global_height(prec=prec) for t in self])

I think more explanation is warranted in the docs as well. i.e., that you are taking the max of the heights of the component points.

and similarly for local height.

@bhutz
Copy link

bhutz commented Jul 13, 2018

Reviewer: Ben Hutz

@vbraun
Copy link
Member

vbraun commented Aug 8, 2018

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