Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#11700 closed enhancement (fixed)

Methods concerning cores in Partitions

Reported by: aschilling Owned by: sage-combinat
Priority: major Milestone: sage-4.7.2
Component: combinatorics Keywords: partitions, cores
Cc: zabrocki@… Merged in: sage-4.7.2.alpha3
Authors: Anne Schilling Reviewers: Mike Zabrocki
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by aschilling)

The patch implements some new methods regarding cores in Partitions.


Apply trac_11700-partitions-as.patch

Attachments (1)

trac_11700-partitions-as.patch (2.3 KB) - added by aschilling 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by aschilling

  • Authors set to Anne Schilling
  • Status changed from new to needs_review

comment:2 follow-up: Changed 6 years ago by zabrocki

  • Status changed from needs_review to needs_work

I ran all tests and they passed. I give it a positive review except for a suggested change to is_core that I believe is shorter and faster:

def is_core( self, k ):
    return not k in self.hooks()

This changes the behavior for k=0 since the original version raises a "ZeroDivisionError?: Integer modulo by zero" while this new version simply returns False.

comment:3 in reply to: ↑ 2 Changed 6 years ago by aschilling

  • Reviewers set to Mike Zabrocki
  • Status changed from needs_work to needs_review

Hi Mike,

Thanks for your very first patch review! The suggested change is made (though for k=0 the output is True and not False).

Anne

comment:4 Changed 6 years ago by zabrocki

  • Status changed from needs_review to positive_review

comment:5 Changed 6 years ago by leif

  • Description modified (diff)

comment:6 follow-up: Changed 6 years ago by nthiery

  • Description modified (diff)

Changed 6 years ago by aschilling

comment:7 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by aschilling

  • Description modified (diff)

Replying to nthiery:

I changed the header of the patch.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by leif

Replying to aschilling:

Replying to nthiery:

I changed the header of the patch.

I.e., just the commit message? (It's not clear to what you replied, Nicolas just removed the hint on the repo I previously added, for whatever reason.)

(I actually already merged the patch into a preliminary alpha3 a few hours ago, before you updated it. But shouldn't be a problem, at least if there are no "real" changes. I'll grab the latest version for an official release anyway.)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by nthiery

Replying to leif:

Replying to aschilling:

I changed the header of the patch.

I.e., just the commit message?

Yup.

It's not clear to what you replied, Nicolas just removed the hint on the repo I previously added, for whatever reason.

The testbot error log was not clear, but hinted that it had not found the patch on the ticket page. So I removed the comment after the "Apply ..." in case this was the cause. Apparently the buildbot now applies the patch (with test failures however). I do not know whether this is just a coincidence or not.

Cheers,

Nicolas

comment:10 in reply to: ↑ 9 Changed 6 years ago by leif

Replying to nthiery:

Replying to leif:

It's not clear to what you replied, Nicolas just removed the hint on the repo I previously added, for whatever reason.

The testbot error log was not clear, but hinted that it had not found the patch on the ticket page. So I removed the comment after the "Apply ..." in case this was the cause.

That was my guess...

So the buildbot deletes some previous logs? The last "apply failed" is from August.

The last statement I recall regarding what the patch-/buildbot inspects was that it doesn't look at the ticket's description at all (but in contrast Jeroen's tools do; just as mine). Haven't kept track on that though.


Apparently the buildbot now applies the patch (with test failures however).

The doctest errors are quite funny; at least one caused by the buildbot itself, most (or all?) of the others caused by "no space left on device". :D

But it IMHO doesn't make much sense to test patches still against 4.7.1 anyway.

comment:11 follow-up: Changed 6 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 in reply to: ↑ 11 Changed 6 years ago by aschilling

Replying to leif:

Sorry, I was out of e-mail contact today. Given the last message, I suppose everything was ok and merged? I only changed the very first line of the patch to

#11700: New methods concerning cores in Partitions

by an e-mail request from Nicolas.

Note: See TracTickets for help on using tickets.