Opened 3 years ago

Closed 3 years ago

#20009 closed enhancement (fixed)

string monoid class one not defined

Reported by: kcrisman Owned by:
Priority: major Milestone: sage-7.1
Component: algebra Keywords:
Cc: Merged in:
Authors: Karan Desai Reviewers: Karl-Dieter Crisman, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: e319260 (Commits) Commit: e319260ec07497b3b3d3276be16131edb3b50ece
Dependencies: Stopgaps:

Description

From this ask.sagemath question:

sage: BinaryStrings().one()

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

Change History (23)

comment:1 Changed 3 years ago by karandesai-96

  • Branch set to u/karandesai-96/string_monoid_class_one_not_defined

comment:2 Changed 3 years ago by karandesai-96

  • Authors set to Karan Desai
  • Branch u/karandesai-96/string_monoid_class_one_not_defined deleted
  • Status changed from new to needs_review

comment:3 Changed 3 years ago by karandesai-96

Hi @kcrisman Does my commit serve the purpose ?

comment:4 Changed 3 years ago by karandesai-96

  • Branch set to u/karandesai-96/string_monoid_class_one_not_defined

comment:5 Changed 3 years ago by kcrisman

  • Commit set to de15718e2fd05af0386baf6ec73fd8556a0b2ffb
  • Reviewers set to Karl-Dieter Crisman

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:

de15718Define one() method in StringMonoid_class

comment:6 Changed 3 years ago by tmonteil

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Thierry Monteil
  • Status changed from needs_review to needs_work

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

comment:7 Changed 3 years ago by git

  • Commit changed from de15718e2fd05af0386baf6ec73fd8556a0b2ffb to 4b9c7394508ba2cb83405fd2a44c05e5b650c7ab

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

4b9c739Change type of StringMonoid_class._identity_element.

comment:8 Changed 3 years ago by karandesai-96

This should do the work.

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

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

comment:9 Changed 3 years ago by tmonteil

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 ?

comment:10 follow-up: Changed 3 years ago by 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.

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

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by tmonteil

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.

comment:12 Changed 3 years ago by git

  • Commit changed from 4b9c7394508ba2cb83405fd2a44c05e5b650c7ab to 546b6ad213c348da6529eb3a96769ef2f0c5b78a

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

648672dFixes StringMonoid_class.one method
546b6adAdd doctests for StringMonoid_class.one()

comment:13 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by karandesai-96

Replying to 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.

comment:14 in reply to: ↑ 13 Changed 3 years ago by tmonteil

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.

comment:15 Changed 3 years ago by karandesai-96

Hi,

Does this ticket need further work ?

comment:16 Changed 3 years ago by karandesai-96

  • Status changed from needs_work to needs_review

comment:17 Changed 3 years ago by karandesai-96

@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.

Last edited 3 years ago by karandesai-96 (previous) (diff)

comment:18 follow-up: Changed 3 years ago by tmonteil

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``.

comment:19 in reply to: ↑ 18 Changed 3 years ago by karandesai-96

Replying to 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. :-)

comment:20 Changed 3 years ago by git

  • Commit changed from 546b6ad213c348da6529eb3a96769ef2f0c5b78a to ebd6276a33ee75a163594cf8b53f746d4597429b

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

ebd6276Change docstring of StringMonoid_class.one()

comment:21 Changed 3 years ago by tmonteil

  • Branch changed from u/karandesai-96/string_monoid_class_one_not_defined to u/tmonteil/string_monoid_class_one_not_defined

comment:22 Changed 3 years ago by tmonteil

  • Commit changed from ebd6276a33ee75a163594cf8b53f746d4597429b to e319260ec07497b3b3d3276be16131edb3b50ece
  • Status changed from needs_review to positive_review

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


New commits:

e319260#20009 reviewer : add a missing dot.

comment:23 Changed 3 years ago by vbraun

  • Branch changed from u/tmonteil/string_monoid_class_one_not_defined to e319260ec07497b3b3d3276be16131edb3b50ece
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.