Opened 5 years ago

Closed 5 years ago

#17232 closed enhancement (fixed)

Remove useless "copy" arguments and normalize steiner_quadruple_system

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.4
Component: combinatorial designs Keywords:
Cc: vdelecroix, dimpase Merged in:
Authors: Nathann Cohen Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 3b43f95 (Commits) Commit: 3b43f95f7711884b5a908f062c3dc378260cc927
Dependencies: Stopgaps:

Description (last modified by ncohen)

With this branch the following is done:

1) The "copy" boolean flag is removed from the methods of IncidenceStructure where it was useless, i.e. all except the constructors. This was always a "don't change unless you know what you do" flag, and persons who know what they do might as well access the internal data structure immediately, i.e. without function call.

2) The file steiner_quadruple_systems did not use IncidenceStructure at all, and this is now fixed. There was also a function is_steiner_quadruple_system which was a specific case of IncidenceStructure.is_t_design. Now, all those functions take IncidenceStructure as input and return IncidenceStructure objects. In particular:

Before

sage: designs.steiner_quadruple_system(20)
((0, 1, 2, 9),
 (0, 1, 12, 19),
 (0, 11, 2, 19),
...

After

sage: designs.steiner_quadruple_system(20)
Incidence structure with 20 points and 285 blocks

Nathann

Change History (11)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/17232
  • Cc vdelecroix dimpase added
  • Commit set to a09f66038c02df4785213b9d40310e91ded13b62
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

a09f660trac #17232: Remove useless "copy" arguments and normalize steiner_quadruple_system

comment:2 Changed 5 years ago by vdelecroix

Hello Nathann,

I updated a branch at u/vdelecroix/17232 which:

  • removes is_steiner_quadruple_system
  • copy the output in the method ground_set (you made as default to not copy the list).
  • change output of _SQS14 and _SQS38 to be lists and use a copy=False in steiner_quadruple_systems for the corresponding parameters.

Do you like it?

Vincent

comment:3 Changed 5 years ago by vdelecroix

  • Status changed from needs_review to needs_info

comment:4 Changed 5 years ago by ncohen

  • Branch changed from u/ncohen/17232 to u/vdelecroix/17232
  • Commit changed from a09f66038c02df4785213b9d40310e91ded13b62 to d19a95bdb2ed9eff8253960b847deeb53b53971f
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_info to positive_review

Thanks for the review !

Nathann


New commits:

d19a95btrac #17232: review

comment:5 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Conflict, most likely #17104

comment:6 Changed 5 years ago by ncohen

  • Branch changed from u/vdelecroix/17232 to public/17232
  • Commit changed from d19a95bdb2ed9eff8253960b847deeb53b53971f to b9140dbaf7ab26a803fd8297293f84938cf3e31d
  • Status changed from needs_work to positive_review

Merged, but I did not see any conflict ...

Nathann


New commits:

c081fd0trac #17104: IncidenceStructure.relabel() (no arguments)
3ade030trac #17104: allow list/tuple + add inplace arg
8615954trac #17104: reviewer comments
b9140dbtrac #17232: Merged with #17104

comment:7 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Which means that the conflict is elsewhere. Wait for the next beta.

comment:8 Changed 5 years ago by git

  • Commit changed from b9140dbaf7ab26a803fd8297293f84938cf3e31d to 3b43f95f7711884b5a908f062c3dc378260cc927

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

3b43f95trac #17232: Merged with rc0

comment:9 follow-up: Changed 5 years ago by vdelecroix

  • Status changed from needs_work to positive_review

tests pass. doc builds.

(I changed the ticket from needs_work to positive_review hoping that it is what you wanted)

Vincent

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

(I changed the ticket from needs_work to positive_review hoping that it is what you wanted)

Thanks !

I was waiting a bit to change the status for I was still building the new rc0 and wanted to check the designs/ folder first.

Well, it just finished... And the tests passed here too :-)

Nathann

comment:11 Changed 5 years ago by vbraun

  • Branch changed from public/17232 to 3b43f95f7711884b5a908f062c3dc378260cc927
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.