string monoid class one not defined

[kcrisman]

From this ask.sagemath question:

sage: BinaryStrings().one()

TypeError: Argument x (= 1) is of the wrong type.

Component: algebra

Author: Karan Desai

Branch/Commit: e319260

Reviewer: Karl-Dieter Crisman, Thierry Monteil

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

[karandesai-96]

Branch: u/karandesai-96/string_monoid_class_one_not_defined

[karandesai-96]

Changed branch from u/karandesai-96/string_monoid_class_one_not_defined to none

[karandesai-96]

Author: Karan Desai

[karandesai-96]

comment:3

Hi @kcrisman
Does my commit serve the purpose ?

[karandesai-96]

Branch: u/karandesai-96/string_monoid_class_one_not_defined

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:5

Looks good to me, but I'm not an expert in such things so I'll let those who are give the final review. I will say that you need a docstring and examples...

---

New commits:

de15718

Define one() method in StringMonoid_class

[kcrisman]

Commit: de15718

[sagetrac-tmonteil]

comment:6

BinaryStrings().one() should be an element of BinaryStrings(), not a Python string. Indeed, you want to be able to write things like:

sage: b = BinaryStrings()
sage: b.one() * b("101") == b("101")
True

[sagetrac-tmonteil]

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Thierry Monteil

[sagetrac-git]

Changed commit from de15718 to 4b9c739

[sagetrac-git]

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

4b9c739

Change type of StringMonoid_class._identity_element.

[karandesai-96]

comment:8

This should do the work.

self._identity_element = StringMonoidElement(self, [int(1)])

This makes it a StringMonoidElement, and not a python string.

[sagetrac-tmonteil]

comment:9

This does not work. As such it returns the second element of the alphabet, as an element of the string monoid:

sage: b = BinaryStrings()
sage: b.one()
1
sage: b.one() * b('001')
1001

sage: b = AlphabeticStrings()
sage: b.one()
B

What we are looking here is the neutral element for the multiplication, which you can get as StringMonoidElement(self, '').

You still need to add some doctests.

Also, why creating a _identity_element attribute while it is used only once, why not directly define the identity element in the one method ?

[karandesai-96]

comment:10

The identity element might be required somewhere else, so I thought let it be declared as a member of the class, though for now I'll move it locally.

Oh, I didn't think about this happening ! I will fix it in a commit quickly.

[sagetrac-tmonteil]

comment:11

Replying to @karandesai-96:

The identity element might be required somewhere else, so I thought let it be declared as a member of the class, though for now I'll move it locally.

Let me suggest not doing things unless you actually need it, since this make some noise in the source code. Also, since no other monoid has such an attribute, it is likely that if someone once needs something like that, she will create an attribute with another name higher in the hierarchy, and the information will be stored twice. I guess that if someone needs the identity element, she will just call self.one() since it is the standard notation for monoids within Sage.

[sagetrac-git]

Changed commit from 4b9c739 to 546b6ad

[sagetrac-git]

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

648672d	Fixes StringMonoid_class.one method
546b6ad	Add doctests for StringMonoid_class.one()

[karandesai-96]

comment:13

Replying to @sagetrac-tmonteil:
Yes, you are correct, I now understand why creating it within one() itself is better.

Also, I am new to development for Sage, I want to ask you whether you build Sage completely again to review my code ? Building Sage takes a lot of time on my laptop, how did you manage to test my code ?

Let me suggest not doing things unless you actually need it, since this make some noise in the source code. Also, since no other monoid has such an attribute, it is likely that if someone once needs something like that, she will create an attribute with another name higher in the hierarchy, and the information will be stored twice. I guess that if someone needs the identity element, she will just call self.one() since it is the standard notation for monoids within Sage.

[sagetrac-tmonteil]

comment:14

Replying to @karandesai-96:

Also, I am new to development for Sage, I want to ask you whether you build Sage completely again to review my code ? Building Sage takes a lot of time on my laptop, how did you manage to test my code ?

Since your branch is based on develop.7.1.beta1 which i compiled already, Sage only had to compile a few files when i type make after applying your branch.

[karandesai-96]

comment:15

Hi,

Does this ticket need further work ?

[karandesai-96]

comment:17

@sagetrac-tmonteil @kcrisman A gentle reminder. Is the current work fit to be merged ? This ticket will go stale and sit here as is for long if not reviewed... :/

Please direct me to further needed work if any, I will do it quickly.

[sagetrac-tmonteil]

comment:18

It looks much better now.

Perhaps could you just replace the current description Create an identity StringMonoid Element, by the more common Return the identity element of ``self``.

[karandesai-96]

comment:19

Replying to @sagetrac-tmonteil:

Perhaps could you just replace the current description Create an identity StringMonoid Element, by the more common Return the identity element of ``self``.

Oh yes, this is better. I will push my __tentatively__ final commit here. :-)

[sagetrac-git]

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

ebd6276

Change docstring of StringMonoid_class.one()

[sagetrac-git]

Changed commit from 546b6ad to ebd6276

[sagetrac-tmonteil]

Changed branch from u/karandesai-96/string_monoid_class_one_not_defined to u/tmonteil/string_monoid_class_one_not_defined

[sagetrac-tmonteil]

Changed commit from ebd6276 to e319260

[sagetrac-tmonteil]

comment:22

I added a missing dot, apart from that it is good to go!

---

New commits:

e319260

#20009 reviewer : add a missing dot.

[vbraun]

Changed branch from u/tmonteil/string_monoid_class_one_not_defined to e319260