Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10025 closed defect (fixed)

sloane_sequence(111776)[2][:36] != sloane.A111776.list(36)

Reported by: donmorrison Owned by: sage-combinat
Priority: minor Milestone: sage-4.6.1
Component: combinatorics Keywords: combinat sloane
Cc: sage-combinat Merged in: sage-4.6.1.alpha0
Authors: Yann Laigle-Chapuy, Nathann Cohen Reviewers: Nathann Cohen, Yann Laigle-Chapuy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

sage: sloane_sequence(111776)[2][:36] != sloane.A111776.list(36)

Searching Sloane's online database...

True

sage: sloane_sequence(111776)[2][:36] == sloane.A111776.list(36)

Searching Sloane's online database...

False

sage: sloane_sequence(111776)[2][:36]

Searching Sloane's online database...

[1, 1, 1, 1, 2, 3, 1, 4, 6, 10, 1, 8, 12, 20, 35, 1, 16, 24, 40, 70, 125, 1, 32, 48, 80, 140, 250, 450, 1, 64, 96, 160, 280, 500, 900, 1625]

sage: sloane.A111776.list(36)

[1, 1, 1, 2, 1, 2, 3, 2, 1, 3, 4, 2, 3, 2, 4, 5, 1, 2, 4, 2, 5, 6, 4, 2, 3, 5, 4, 6, 7, 2, 5, 2, 1, 6, 4, 7]

sage: len(sloane_sequence(111776)[2][:36]) == len(sloane.A111776.list(36))

Searching Sloane's online database...

True

Attachments (2)

trac10025-correct_sloane_sequences.patch (6.8 KB) - added by ylchapuy 8 years ago.
trac_10025-smallfixes.patch (4.7 KB) - added by ncohen 8 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by drkirkby

  • Authors Jaap Spies, Nick Alexander deleted
  • Report Upstream changed from Reported upstream. Little or no feedback. to N/A

A couple of points

  • The Authors field is used to indicate who has written code to patch a problem, or new code to add functionality - not who wrote the original code. So at this point in time, since this is just a bug report, with no patch, the Author's field should be blank.
  • The Reported upstream is used only when the bug is in code that's not directly maintained by Sage. So bugs in Python, GSL, ATLAS etc which are found should be reported upstream.

Changed 8 years ago by ylchapuy

comment:2 Changed 8 years ago by ylchapuy

  • Status changed from new to needs_review

The given patch does the following:

  • rename A111776 to it's correct name A109814
  • correct the discrepancy with the online version
  • correct an offset problem with A000272 (there *is* one tree with no node)
  • add an optional long doctest requiring internet to test tha sage keeps on par with OEIS

comment:3 Changed 8 years ago by ylchapuy

  • Authors set to Yann Laigle-Chapuy
  • Milestone changed from sage-4.6.1 to sage-4.6

comment:4 Changed 8 years ago by ylchapuy

  • Cc sage-combinat added

comment:5 Changed 8 years ago by ylchapuy

  • Milestone changed from sage-4.6 to sage-4.6.1

4.6 is feature freezed...

comment:6 Changed 8 years ago by ncohen

Hello !!!

Here is another patch to add to yours... The doctests did not pass, as the sloane_sequence method always outputs "Searching Sloane's online database...". I tried to find ways around, then thought the best was to just let it forward a verbose parameter to find_sloane... Anyway, I made this modification, and I also moved most of the documentation written in the init method of class A109814 just after the class definition. This way, it appears in the documentation when it is built.

The same should be done for the other methods, as currently very few of them appear : http://www.sagemath.org/doc/reference/sage/combinat/sloane_functions.html

Only my patch needs done to be reviewed before this ticket can be set as positively reviewed ! :-)

Nathann

Changed 8 years ago by ncohen

comment:7 follow-up: Changed 8 years ago by drkirkby

  • Status changed from needs_review to needs_work

What version of Sage is your patch against? I get problems if I try to apply to sage-4.6.rc0.

drkirkby@hawk:~/sage-4.6.rc0/devel/sage-main$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10025/trac_10025-smallfixes.patch
adding trac_10025-smallfixes.patch to series file
drkirkby@hawk:~/sage-4.6.rc0/devel/sage-main$ hg qpush
applying trac_10025-smallfixes.patch
patching file sage/combinat/sloane_functions.py
Hunk #1 FAILED at 49
Hunk #2 FAILED at 8992
Hunk #3 FAILED at 9031
3 out of 3 hunks FAILED -- saving rejects to file sage/combinat/sloane_functions.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_10025-smallfixes.patch
drkirkby@hawk:~/sage-4.6.rc0/devel/sage-main$ 

comment:8 in reply to: ↑ 7 Changed 8 years ago by ncohen

Replying to drkirkby:

What version of Sage is your patch against? I get problems if I try to apply to sage-4.6.rc0.

Well, I am working on 4.6.rc0 too... Though both patch need to be applied !

Nathann

comment:9 follow-up: Changed 8 years ago by drkirkby

Yes, i missed the first patch needed to be applied first.

Should this not return an error, rather than waste bandwidth looking on the database?

sage: sloane_find(32.23)
Searching Sloane's online database...
[]

I's also puzzled that

sage: sloane_find(sin(x)) # where I have not defined x
Searching Sloane's online database...

should actually produce any output, but it does.

I'm not really into OEIS, so perhaps I'm mistaken.

Dave

comment:10 in reply to: ↑ 9 Changed 8 years ago by ncohen

  • Status changed from needs_work to needs_review

Replying to drkirkby:

I's also puzzled that

sage: sloane_find(sin(x)) # where I have not defined x
Searching Sloane's online database...

Well, by default x is a symbolic variable, so "sin(x)" has some meaning already... Later on, I think the argument gets translated into a string, which still has meaning, and there shouldn't be anything wrong until the website answers that there is no sequence corresponding to this non-integer.

To be honest I had used this method once or twice only since I know Sage, and I already did not like the fact that it could not be told to stay mute (and also that verbosity is the default behaviour). Well, it's getting slowly fixed :-)

Nathann

comment:11 Changed 8 years ago by ylchapuy

Hi, Here, the primary goal is to correct the fact that sloane.A111776 is in fact the sequence A109814. I also added a test to check that be sure this doesn't happen again, which test every sequences against the online version. With respect to this, I'm happy with the changes made by Nathann and would give a positive review.

I do agree that it's a bad thing not to test the input for correctness, but it's not the point of this ticket. David, please feel free to open another one to adress this issue. I also let you make the decision about giving a positive review or not, as I'm a little bit biased as an author.

cheers,

Yann

comment:12 Changed 8 years ago by drkirkby

I think it is better that you and Nathan review each others patches, as I don't consider myself qualified to check this - my pthon skills are not good. There is no problem in having two authors and two reviewers, as long as they review each others work, and not their own.

I guess I should open a ticket for the fact the code does not check its input, but there are more serious cases. Try

sage: seed(1,2)

for example.

Dave

comment:13 Changed 8 years ago by ylchapuy

  • Authors changed from Yann Laigle-Chapuy to Yann Laigle-Chapuy, Nathann Cohen
  • Reviewers set to Nathann Cohen, Yann Laigle-Chapuy
  • Status changed from needs_review to positive_review

comment:14 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 8 years ago by was

Related note: sloane_find is now completely broken. See #10358.

Note: See TracTickets for help on using tickets.