Opened 11 years ago
Closed 10 years ago
#11863 closed enhancement (fixed)
Bilinear Map
Reported by: | Anna Haensch | Owned by: | Justin Walker |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.9 |
Component: | quadratic forms | Keywords: | |
Cc: | Merged in: | sage-5.9.beta0 | |
Authors: | Anna Haensch, Frédéric Chapoton | Reviewers: | Kannappan Sampath |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This function evaluates two vectors in a quadratic space under the bilinear map associated to the space.
This is sort of a trivial function, and it may exist already...I couldn't find it though. It is mostly important because it simplifies the implementation of other quadratic forms functions, such as Ticket #11841.
Attachments (4)
Change History (22)
Changed 11 years ago by
Attachment: | bilinear_map.patch added |
---|
comment:1 Changed 11 years ago by
Component: | PLEASE CHANGE → quadratic forms |
---|---|
Owner: | changed from tbd to Justin Walker |
Type: | PLEASE CHANGE → enhancement |
Changed 11 years ago by
Attachment: | Trac_11863.patch added |
---|
comment:2 Changed 11 years ago by
comment:3 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | new → needs_review |
comment:4 Changed 10 years ago by
Status: | needs_review → needs_info |
---|
AFAI looked, this is not implemented. And, indeed, it would be a very useful addition for implementing a lot of other things. But, I do have a couple of comments:
- I am OK with this method being implemented in
quadratic_forms.py
. But, do people think that, this method should be added to the Cython-basedquadratic_form__evaluate.pyx
? (I am guessing based on the name of the file.)
About the patch:
- The docstring now reads: "Given a quadratic form
Q', this gives the image of two vectors in
Q` under the associated bilinear map."
- As you can see the docstring formatting is messed up: The first Q should be enclosed by backticks on either side as opposed to a single quote. Sorry about the minor nitpick.
- The vectors are in the base ring and not "in
Q
". A minor typo here. :) - Could you please define what the "associated bilinear form" is?
- From the implementation, I think we don't want the ring to be of characteristic 2, as
1/2
would not make sense in such a ring. How are we going to take care of this? (ring has acharacteristic()
method...)
- Could you please add a space between the docstring and the INPUTS?
Cheers, Kannappan.
comment:5 Changed 10 years ago by
Reviewers: | → Kannappan Sampath |
---|
Changed 10 years ago by
Attachment: | Trac_11863v2.patch added |
---|
comment:6 follow-up: 7 Changed 10 years ago by
Thanks for the comments Kannappan!
I fixed up the errors in the documentation, thanks for spotting that characteristic 2 error, that's an important one. I chose to leave the function in the quadratic_forms.py folder.
Perhaps you could run some doctests and coverage tests for me if you get the chance...or maybe you already did?
Best, Anna
comment:7 Changed 10 years ago by
Replying to annahaensch: Hello Anna!
Thanks for the comments Kannappan!
I fixed up the errors in the documentation, thanks for spotting that characteristic 2 error, that's an important one. I chose to leave the function in the quadratic_forms.py folder.
Looks better.
- The vectors are still from
Q
but their entries should be really from the base ring ofQ
(they must come frombase_ring^no_of_variables
). - I just have one more question: most books on Quadratic forms (eg. Scharlau's) seem to prefer without dividing by 2. Do you think we must really divide by 2?
- And, they also call this the polar form corresponding to the given quadratic form. So, how about an alias:
polar_form
?
- And, they also call this the polar form corresponding to the given quadratic form. So, how about an alias:
- Also, given a bilinear form, we could get a quadratic form in a canonical way. Is this implemented?
Apart from this, we could set this to positive review.
Perhaps you could run some doctests and coverage tests for me if you get the chance...or maybe you already did?
I did! I have come up with a couple of issues that we need to fix.
- First, the look of the quadratic_form.html - I shall open a ticket for this.
- Second, there is a failing doctest in equivalence testing: #14093.
Could you please review this?
- Thirdly, the file automorphisms seems to have a couple of issues, I have not yet worked out. I have posted on sage-devel. Please do look. You may have some comments.
~KnS
comment:8 follow-up: 9 Changed 10 years ago by
You should raise TypeError? and not return them.
raise TypeError("Bilinear form can only be evaluated on vectors with length " + str(self.dim()) + ".")
and
raise TypeError("Not defined for rings of characteristic 2.")
There should also be a doctest for the second raise error.
Please look at http://www.sagemath.org/doc/developer/conventions.html for the syntax of INPUT and OUTPUT fields (no indentation needed)
comment:9 Changed 10 years ago by
Replying to chapoton:
Hello Chapoton,
raise TypeError("Not defined for rings of characteristic 2.")
The Whole point of my comments in the previous message, was whether we should divide by 2 at all. Scharlau, for example does not seem to require that.
Changed 10 years ago by
Attachment: | trac_11863_bilinear_form_v3.patch added |
---|
comment:10 Changed 10 years ago by
Description: | modified (diff) |
---|
Here is a clean patch, to use as new starting point for further discussions. There still remains the question about 2.
For the bot:
apply trac_11863_bilinear_form_v3.patch
comment:11 follow-up: 12 Changed 10 years ago by
Chapoton, thank you for the updated patch, that looks quite good.
Regarding the question about 2, I am following the conventions set forth in O'meara, for which we require a field of characteristic not 2. This is different from the conventions used in Scharlau's work, but is equally standard.
Also, given a bilinear form, we could get a quadratic form in a canonical way. Is this implemented?
Yes, we can! I'm not sure if it's implemented. Certainly it would be an issue separate from this function, since this uses the quadratic form to define the bilinear. But that is something to consider in the future.
comment:12 Changed 10 years ago by
Replying to annahaensch:
Hello Anna,
Regarding the question about 2, I am following the conventions set forth in O'meara, for which we require a field of characteristic not 2. This is different from the conventions used in Scharlau's work, but is equally standard.
Anna, I sincerely hope you're not offended! I am absolutely fine with both of these conventions. I am convinced that this should be given positive review now! If you agree (could chapoton and you please fill in your real name as author), I shall set this to positive review.
Yes, we can! I'm not sure if it's implemented. Certainly it would be an issue separate from this function, since this uses the quadratic form to define the bilinear. But that is something to consider in the future.
Agreed! I shall open a new ticket.
comment:13 Changed 10 years ago by
Authors: | → Anna Haensch |
---|
comment:14 Changed 10 years ago by
Knsam, I'm not the least bit offended :) I sincerely appreciate your help and input on this patch. I feel like a positive review would be very good at this point, please proceed with that. Thanks!
comment:15 Changed 10 years ago by
Authors: | Anna Haensch → Anna Haensch, Frédéric Chapoton |
---|---|
Status: | needs_info → needs_review |
comment:16 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
comment:17 Changed 10 years ago by
Milestone: | sage-5.8 → sage-5.9 |
---|
comment:18 Changed 10 years ago by
Merged in: | → sage-5.9.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Trac_11863.patch is the same as bilinear_map.patch, I just wanted to stick to the naming convention. Sorry about that.