Opened 5 years ago
Last modified 4 years ago
#20936 needs_work enhancement
Adding conversion for Divide and Product class in Sage/M2 interface
Reported by:  zonova  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  interfaces  Keywords:  Macaulay2, interface 
Cc:  Merged in:  
Authors:  Saad Khalid  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/zonova/adding_conversion_for_divide_and_product_class_in_sage_m2_interface (Commits, GitHub, GitLab)  Commit:  a5601a221335cc3dca433795d08015ae5aa21ca1 
Dependencies:  Stopgaps: 
Description
This adds the method sage_prodstring(), which is similar to sage_polystring, except that it is for properly converting the macaulay2 Product object to a string following python's math programming conventions, so that I could add conversion of Product objects from M2 in the to_sage() function
There were also additions to the to_sage() function. I added an elif statement for converting Product objects, and an elif for converting Divide objects from M2 to Sage. Both of their outputs are as strings.
Attachments (1)
Change History (24)
Changed 5 years ago by
comment:1 Changed 5 years ago by
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
 Branch set to u/zonova/adding_conversion_for_divide_and_product_class_in_sage_m2_interface
comment:4 Changed 5 years ago by
 Commit set to fc74c1c2fafa874e3008d2ae6402e8b469c3518d
Does that work?
New commits:
fc74c1c  adding conversion for Product and Divide class in M2/Sage interface.

comment:5 Changed 5 years ago by
Why do you remove from __future__ import print_function
and revert Python3compatible print
statements back to old form? Did you work with an old file?
You should merge your work with the current beta.
comment:6 Changed 5 years ago by
 Branch u/zonova/adding_conversion_for_divide_and_product_class_in_sage_m2_interface deleted
 Commit fc74c1c2fafa874e3008d2ae6402e8b469c3518d deleted
comment:7 Changed 5 years ago by
 Branch set to u/zonova/adding_conversion_for_divide_and_product_class_in_sage_m2_interface
 Commit set to fc74c1c2fafa874e3008d2ae6402e8b469c3518d
New commits:
fc74c1c  adding conversion for Product and Divide class in M2/Sage interface.

comment:8 Changed 5 years ago by
 Commit changed from fc74c1c2fafa874e3008d2ae6402e8b469c3518d to 411b8bd64bb089a9a347247f22d1d932120d0a5e
Branch pushed to git repo; I updated commit sha1. New commits:
11676ab  updated my files and then redid the whole Product and Divide thing.

1cb18fe  Merge branch 'u/zonova/adding_conversion_for_divide_and_product_class_in_sage_m2_interface' of git://trac.sagemath.org/sage into t/20936/adding_conversion_for_divide_and_product_class_in_sage_m2_interface

411b8bd  getting the macaulay2.py file back in

comment:9 Changed 5 years ago by
 Commit changed from 411b8bd64bb089a9a347247f22d1d932120d0a5e to af1b40d505b981446ad79e76c28554a6759145aa
Branch pushed to git repo; I updated commit sha1. New commits:
af1b40d  fixing macaulay2 interface file to most recent vs of sage and then

comment:10 Changed 5 years ago by
After a bunch of git based atrocities(my own lack of experience), I think I finally fixed it. Does it look okay? Do I need to add more comments anywhere? I didn't know if I had to include my name with the edit in the comments. Thanks for the patience.
comment:11 Changed 5 years ago by
 Status changed from needs_review to needs_work
You should test your changes using Sage's doctesting framework. (sage t
). With your branch I get
sage t src/sage/interfaces/macaulay2.py Error: TAB character found at lines 840,856,857,858,1190 [28 tests, 0.04 s]  sage t src/sage/interfaces/macaulay2.py # Tab character found
Do not use tabs, only use spaces for formatting (it should be easy to tell your editor to map tab key to 4 spaces).
comment:12 Changed 5 years ago by
 Commit changed from af1b40d505b981446ad79e76c28554a6759145aa to a5601a221335cc3dca433795d08015ae5aa21ca1
Branch pushed to git repo; I updated commit sha1. New commits:
a5601a2  fixing tabs to 4 spaces

comment:13 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 5 years ago by
How does it look?
comment:15 followup: ↓ 20 Changed 5 years ago by
It seems you have broken some working parts, e.g.:
sage: macaulay2('5/4').to_sage() 5/4 sage: type(macaulay2('5/4').to_sage()) <type 'sage.rings.rational.Rational'>
makes perfect sense, while your patch makes this result a string.
comment:16 Changed 5 years ago by
So, I've realized that M2 has a function, toString, that can be used to convert the ASCII art version of a polynomial into a polynomial following python's math syntax. Given this new knowledge, do you think that I should just delete this branch and perhaps later try writing a version that converts objects of the Divide and Product class into elements of SR in Sage? Or would it still be worth it to fix this version that converts it to strings? I can't think of a reason why, but if you can think of one, I'm happy to do it.
comment:17 Changed 5 years ago by
one way or another, I don't see why you ever want an interface to Sage SR objects rather than to its multivariate polynomial arithmetic. E.g. you would be stuck if on M2 side the Divide or the Product object you want to convert is defined over a field of positive characteristic, something that SR does not handle.
comment:18 followup: ↓ 19 Changed 5 years ago by
Hmm, okay. How would I check if the object is defined over a field of positive characteristic in Macaulay? Or, would converting it into a multivariate polynomial do that automatically? I guess my question is, why does it work with MPA but not with SR?
comment:19 in reply to: ↑ 18 Changed 5 years ago by
Replying to zonova:
Hmm, okay. How would I check if the object is defined over a field of positive characteristic in Macaulay? Or, would converting it into a multivariate polynomial do that automatically? I guess my question is, why does it work with MPA but not with SR?
SR only works in characteristic 0, by design. Why do you even need SR?
comment:20 in reply to: ↑ 15 Changed 5 years ago by
I was basing it off of this: https://groups.google.com/forum/#!topic/sagesupport/oVXR3fURslY
But, you are right, it would make more sense to leave it at the first two steps you show in that post rather than changing it to SR. I had just assumed that using SR would have made it easier to treat it as a function, but that is likely my ignorance.
Replying to dimpase:
It seems you have broken some working parts, e.g.:
sage: macaulay2('5/4').to_sage() 5/4 sage: type(macaulay2('5/4').to_sage()) <type 'sage.rings.rational.Rational'>makes perfect sense, while your patch makes this result a string.
I don't understand why rationals like that have cls_str == "Divide", instead of having cls_str == "QQ"... The numerator and denomenator don't have properly defined repr_str's. Should I simply make run a search for a variable if it has cls_Str == "Divide", to differentiate between rational polynomials and rational numbers?
comment:21 Changed 5 years ago by
Also, bigger question, how exactly is a rational function supposed to be a polynomial? Because, if it's a Divide type object in M2, then the numerator and denominator may be polynomials, but the whole thing certainly isn't, as far as I know...
comment:22 Changed 5 years ago by
of course a rational function need not be a polynomial. Here is an example:
sage: R.<x,y>=GF(5)[] sage: x^2/y x^2/y sage: type(x^2/y) <type 'sage.rings.fraction_field_element.FractionFieldElement'> sage: R.fraction_field() Fraction Field of Multivariate Polynomial Ring in x, y over Finite Field of size 5 sage: x^2 in R.fraction_field() True sage: x^2/y in R.fraction_field() True sage: x^2/y in R False
so you get two polynomials p and q from M2, numerator and denominator of a Divide object, p/q, construct in Sage a polynomial ring R containing p and q, and then p/q is an element of R.fractional_field(). This is the plan.
comment:23 Changed 4 years ago by
 Status changed from needs_review to needs_work
indentation is incorrect in the new method.
I think you should create a proper git branch, based on the current beta, rather than this.