Opened 5 years ago

Closed 5 years ago

#16598 closed enhancement (fixed)

New empty design classes for a better user interface and new is_group_divisible_design Cython function

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

Description

This branch defines:

  • GroupDivisibleDesign
  • PairwiseBalancedDesign
  • BalancedIncompleteBlockDesign
  • TransversalDesign

All these classes (which inherit from IncidenceStructure) make it easier to do things like compute the automorphism_group of a transversal design.

The branch also adds a function is_group_divisible_design in designs_pyx.pyx, and an empty is_pairwise_balanced_design that call it. We can thus get rid of the ugly _check_pbd.

Nathann

Change History (54)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16553
  • Commit set to ff72248b89697ebc63ce170e600b3b7764c7c178
  • Dependencies set to #16553
  • Status changed from new to needs_review

New commits:

ff72248trac #16553: Clean IncidenceStructure

comment:2 Changed 5 years ago by ncohen

  • Branch changed from u/ncohen/16553 to u/ncohen/16598
  • Commit changed from ff72248b89697ebc63ce170e600b3b7764c7c178 to 1575f3131e0663b699d9c0a35b0afb88912d7a7a

New commits:

4dc250atrac #16553: Clean IncidenceStructure
bd609fctrac #16553: is_t_design
11994eftrac #16553: Review
e7a97eatrac #16553: doc fix + deprecation is_block_design
3c0dd71trac #16553v3: change .points() -> .ground_set()
52b7177trac #16553: merge sage 6.3.beta5
0698433trac #16553: deprecated alias .points() + fix
1575f31trac #16598: Useless new classes and a replacement for _check_pbd

comment:3 Changed 5 years ago by git

  • Commit changed from 1575f3131e0663b699d9c0a35b0afb88912d7a7a to cdaf1e2e834f3895d1fb096a139446bf8620d319

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cdaf1e2trac #16598: Useless new classes and a replacement for _check_pbd

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

  • Status changed from needs_review to needs_work

Hey Nathann,

In group divisible design, there must be more parameters:

  • v the size of the ground set
  • K the admissible size of the blocks
  • G the admissible size of the groups

The definition seems to be standard (see the big paper of Hanani and the definition IV.1.3 of the Handbook)

Anyway, GDD is just a strict generalization of PBD and it is very clear from Handbook IV.1.3. That way, your is_pbd would just be a lambda function.

Actually, it would be nicer that IncidenceStructure would actually be a GroupDivisibleDesign (groups being the partition into points). The points can be ordered in such way that the groups are

0 1 ... g1-1|g1 g1+1 ... g1+g2-1|...

and then the new class would just need an extra ._group_sizes attribute that would be an integer partition.

Some methods are not well adapted to group divisible designs

  • dual: is the dual well defined for GDD?
  • incidence_matrix: must be the incidence matrix group/block (or put a flag allowing to have points/blocks)
  • automorphis_group is wrong since it must preserver the groups

Why do you use tuples where everywhere in IncidenceStructure there are lists?

Would you mind having a "stupid" .group_sizes() method?

Vincent

comment:5 Changed 5 years ago by vdelecroix

Note that a solution for computing the automorphism group is as follows:

  1. compute the automorphism group of the partition into groups (the generators can be provided by hand, and I can provide a function for it)
  2. compute the automorphism group of the block design (ie forget about the groups)
  3. make the intersection

I think that it is not that bad in terms of efficiency.

Vincent

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by ncohen

Yo !

In group divisible design, there must be more parameters:

  • v the size of the ground set
  • K the admissible size of the blocks
  • G the admissible size of the groups

The definition seems to be standard (see the big paper of Hanani and the definition IV.1.3 of the Handbook)

Seems more than we need at the moment, but okay ...

Anyway, GDD is just a strict generalization of PBD and it is very clear from Handbook IV.1.3. That way, your is_pbd would just be a lambda function.

lambda functions have no documentation. But yes, the code just takes one line.

Actually, it would be nicer that IncidenceStructure would actually be a GroupDivisibleDesign (groups being the partition into points). The points can be ordered in such way that the groups are

0 1 ... g1-1|g1 g1+1 ... g1+g2-1|...

and then the new class would just need an extra ._group_sizes attribute that would be an integer partition.

Until we want GDD to be defined on something different than 0,...,n-1 just like you did for IncidenceStructure

Some methods are not well adapted to group divisible designs

  • dual: is the dual well defined for GDD?

Given that the GDD is a set of blocks, why not ? Plus you wanted two lines above that GDD be equal to an IncidenceStructure.

  • incidence_matrix: must be the incidence matrix group/block (or put a flag allowing to have points/blocks)

Why ?

  • automorphis_group is wrong since it must preserver the groups

Then the implementation is correct.

Why do you use tuples where everywhere in IncidenceStructure there are lists?

Would you mind having a "stupid" .group_sizes() method?

Yes, I hate those methods. Write it if you want one.

Nathann

comment:7 Changed 5 years ago by git

  • Commit changed from cdaf1e2e834f3895d1fb096a139446bf8620d319 to 3f8cebb0d68d7e7ae0e9c763e9bc78a1bdb847dd

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

3f8cebbtrac #16598: G and K as arguments of GDD

comment:8 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

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

Then the implementation is correct.

i.e. an automorphism send a non-covered pair of vertices u,v on another non-covered pair of vertices u,v. So a group is sent on another group with the same cardinality.

It corresponds to the notion of automorphism group you expect for BIBD (K=[1]), and it also corresponds to the notion of group that you expect on transversal designs (K=[k]).

Nathann

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

  • Status changed from needs_review to needs_info

I got it for the automorphism group! Thanks. But your definition of gdd is wrong: any pair of points is either in a group or in lambda blocks but not both. And this is why the blocks completely determine the groups: two points are in the same group if they are not in a block!

What do you think of having only one class (with blocks and groups) and methods:

  • .is_t_design(self,t,v,k,l)
  • .is_group_divisible_design(self,v,G,K,l)
  • .is_partially_balanced_design(self,v,K,l)

I do not like the fact of having one extra empty class whose only purpose is a check at creation... Moreover, if we switch to mutable classes it would be cool that after transformations we check the kind of thing we obtain. The only thing that bother me a little bit is this dual method which does not make any sense for groups (perhaps it does??).

Why do you not translate the groups over integers as it is the case for blocks?

You really like to use tuple of tuples for groups and list of lists for blocks (see comment:4 above)?

Vincent

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

  • Status changed from needs_info to needs_review

Hello !

I got it for the automorphism group! Thanks. But your definition of gdd is wrong:

Ahahahahah. Well, that's what I intended, but... is the new version clearer ?

What do you think of having only one class (with blocks and groups) and methods:

  • .is_t_design(self,t,v,k,l)
  • .is_group_divisible_design(self,v,G,K,l)
  • .is_partially_balanced_design(self,v,K,l)

How would you call the class ? If you want to have these functions as methods of something, the only choice I see is IncidenceStructure. We would have no .groups() method but those functions could get the groups as an argument.

I do not like the fact of having one extra empty class whose only purpose is a check at creation...

I just created GDD because it was the common generalization of TD and PBD. Mostly for the doc at the moment, and we will have some place to put code that works on groups.

Moreover, if we switch to mutable classes it would be cool that after transformations we check the kind of thing we obtain.

Hmmmm... I don't expect that GDD or anything that inherits from it will ever be mutable. IncidenceStructure could be mutable someday, though. It wouldn't be cool to have a GDD object which is not a GDD. Plus as you noticed, most of these objects are created from others, rarely modified inplace.

The only thing that bother me a little bit is this dual method which does not make any sense for groups (perhaps it does??).

Well, it just does not consider them. To me those "groups" are a parameter of the design, nothing else... The dual of a dual of a GDD is a GDD, so everything is cool. The blocks are forgotten (they can be re-computed automatically anyway), but who cares ? I don't think that you would find any other definition of the dual of a GDD anywhere...

Why do you not translate the groups over integers as it is the case for blocks?

Oh. I thought that I would only define GDD for integer ground set, but indeed it also handles the non-integer case almost for free as we know that "._blocks" are already translated to integers. You are right. I will updated that in a second.

You really like to use tuple of tuples for groups and list of lists for blocks (see comment:4 above)?

I don't understand. .groups() returns lists at the moment (before my commit)

def groups(self):
    return map(list,self._groups)

Oh, I see ! You meant for internal storage ! Fixed in the new commit.

Nathann

P.S. : While writing the commit, I made IncidenceStructure mutable. But it's not reaaaaaaaaaaallly a problem, it preserves all the structure ! :-D

comment:12 Changed 5 years ago by git

  • Commit changed from 3f8cebb0d68d7e7ae0e9c763e9bc78a1bdb847dd to fa3c715732a965786f86a2a0ce7087c63294519a

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

fa3c715trac #16598: Relabel, review, doc fix, ...

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

Hi Nathann,

Groups are not a parameter. If you have a GDD, the group are completely determined by the blocks: define the groups in such way that two points are in the same group if and only if they do not belong to a common block.

So, it makes sense to have .is_gdd() at the level of IncidenceStructure and make it returns the groups if needed. We can also have a method .groups() on IncidenceStructure without any attribute ._groups defined.

What do you think?

Vincent

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by ncohen

Groups are not a parameter.

v and k are not a parameter of a BIBD either, they can be deduced. Same for K in PBD.

If you have a GDD, the group are completely determined by the blocks: define the groups in such way that two points are in the same group if and only if they do not belong to a common block.

I know, I mentionned it in my last comment....

So, it makes sense to have .is_gdd() at the level of IncidenceStructure and make it returns the groups if needed. We can also have a method .groups() on IncidenceStructure without any attribute ._groups defined.

What do you think?

I don't like it, you cannot define "groups" properly at this level. What do you think the "groups" of IncidenceStructure(3,[[1,2]]) should be ?

Nathann

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by vdelecroix

Replying to ncohen:

So, it makes sense to have .is_gdd() at the level of IncidenceStructure and make it returns the groups if needed. We can also have a method .groups() on IncidenceStructure without any attribute ._groups defined.

What do you think?

I don't like it, you cannot define "groups" properly at this level. What do you think the "groups" of IncidenceStructure(3,[[1,2]]) should be ?

All right, add the extra assumption that each point is covered by at least one block! If not .groups() raises an exception.

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:16 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by ncohen

All right, add the extra assumption that each point is covered by at least one block! If not .groups() raises an exception.

It's unrelated. What are the blocks of IncidenceStructure(4,[[0,1],[1,2],[2,3]]) ? I am just telling you that {"u~v" if the pair u,v is not covered by a block} is not an equivalence relation unless you are in a GDD.

Nathann

Last edited 5 years ago by ncohen (previous) (diff)

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by vdelecroix

Replying to ncohen:

All right, add the extra assumption that each point is covered by at least one block! If not .groups() raises an exception.

It's unrelated. What are the blocks of IncidenceStructure(4,[[0,1],[1,2],[2,3]]) ? I am just telling you that {"u~v" if the pair u,v is not covered by a block} is not an equivalence relation unless you are in a GDD.

For GDD you have an extra assumption of the existence of a lambda:

blocks = [[0,1],[0,2]]
groups = [[0],[1,2]]

is not a GDD (0 is covered by two blocks, but 1 and 2 are covered by one block). But "not being in a block" for pairs is an equivalence relation...

I definitely agree that "not being in the same block" is not an equivalence relation for many incidence structures (lack of transitivity)... but it is well defined and symmetric.

Vincent

comment:18 in reply to: ↑ 17 Changed 5 years ago by ncohen

For GDD you have an extra assumption of the existence of a lambda:

blocks = [[0,1],[0,2]]
groups = [[0],[1,2]]

is not a GDD (0 is covered by two blocks, but 1 and 2 are covered by one block). But "not being in a block" for pairs is an equivalence relation...

Since when do you need to be regular to be a GDD ? Even PBD are not necessarily regular ! Lambda is about pairs of vertices.

Nathann

comment:19 Changed 5 years ago by git

  • Commit changed from fa3c715732a965786f86a2a0ce7087c63294519a to 1a5a4b8203a2ad104d617a150e2029dbe3a315b1

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

1a5a4b8trac #16598: The constructors implicitly assumed that the ground set was made of integers

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

  • Status changed from needs_review to needs_work

Hi Nathann,

I added a little commit of cleaning (and also replace the deepcopy by something more explicit) at u/vdelecroix/16598.

The semantic of copy=True/False is currently not the same with .blocks() and .groups(). I decided (and you did not complain) that with copy=False it just returns a pointer to the attribute ._blocks which means something on the integers... whatever the ground set could be. If you think it is bad, then we need to change it in .blocks() and .points(). Otherwise, change it in .groups().

In the function is_group_divisible_design it is assumed that the ground set is {0, ..., v-1} (which is just find since it is called with the internal data from GroupDivisibleDesign). It would be cool to specify it in the doc...

What is the point of matrix[i*n+j] != <unsigned short> -1 (in is_group_divisible_design)?

Vincent

comment:21 in reply to: ↑ 20 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

I added a little commit of cleaning (and also replace the deepcopy by something more explicit) at u/vdelecroix/16598.

Looks good !

The semantic of copy=True/False is currently not the same with .blocks() and .groups(). I decided [...]

Oh, cool ! Actually I thought it was written the way I first wrote .groups and I thought that it was not the correct way to implement it, and so I implemented it this way to follow what I thought was the previou choice. I prefer it this way too ! Fixed.

In the function is_group_divisible_design it is assumed that the ground set is {0, ..., v-1} (which is just find since it is called with the internal data from GroupDivisibleDesign). It would be cool to specify it in the doc...

It was specified in the documentation of the mandatory argument v, but I added another occurrence in the one-line description of the function

What is the point of matrix[i*n+j] != <unsigned short> -1 (in is_group_divisible_design)?

Leftover from a previous implementation. Removed !

Nathann

comment:22 Changed 5 years ago by git

  • Commit changed from 1a5a4b8203a2ad104d617a150e2029dbe3a315b1 to 494d82657000af8d4361a630035227c6aab6f5de

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

f454c82trac #16598: reviewer
494d826trac #16598: Reviewer's comments

comment:23 Changed 5 years ago by novoselt

  • Summary changed from Useless new classes and a replacement for _check_pbd to Useful classes for designs and a replacement for _check_pbd

comment:24 Changed 5 years ago by ncohen

O_o

comment:25 Changed 5 years ago by ncohen

  • Summary changed from Useful classes for designs and a replacement for _check_pbd to Useless new design classes and a replacement for _check_pbd

Hey man, I still name my tickets how I see fit, can't I ? O_o

comment:26 follow-up: Changed 5 years ago by novoselt

Of course you can - you just did it and the system didn't stop you, since it's brainless.

This is however not your ticket, it is a ticket (authored by you) intended to be merged into a public collaborative project, where it is helpful to have meaningful titles/descriptions/commits. If the title is reflective of this ticket, I am ready to set it to positive review as invalid/wontfix.

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

Hopefully, humans are not computers and they do understand humour... if you base your reviews on (first degree interpretation of) title of tickets I am not sure it is only the system which is brainless.

comment:28 in reply to: ↑ 26 Changed 5 years ago by ncohen

It seems that one cannot have an enjoyable conversation with anybody, these days ... I don't get it. It cannot be the weather, it is summer !

Nathann

comment:29 in reply to: ↑ 27 ; follow-up: Changed 5 years ago by novoselt

Replying to vdelecroix:

Hopefully, humans are not computers and they do understand humour... if you base your reviews on (first degree interpretation of) title of tickets I am not sure it is only the system which is brainless.

Yes, and when these smart humans look at changelogs to get a sense of new functionality, of course they don't stop there: they open each ticket, read the description, the discussion, and the source code of the branch. And who cares about commit messages when it is so easy to look at diff for anything and make your own judgment unbiased by authors humour?

Or perhaps I am wrong all the way from the start and nobody cares about new functionality or history, so it absolutely does not matter what one can or cannot see in changelogs.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 5 years ago by ncohen

Or perhaps I am wrong all the way from the start and nobody cares about new functionality or history, so it absolutely does not matter what one can or cannot see in changelogs.

You are wrong if you believe that you can edit somebody else's ticket without asking for their opinion first. This is not a software development issue, this is called common sense.

Now, as you can see that you do not bring anything to the work going on here, let's stop this. Or continue by email/sage-devel if you like, but let's not polute this trac ticket.

The same "smart humans" you mentionned like to read the trac tickets message by message when they are done with reading the changelog, and we are just wasting their valuable time.

Nathann

comment:31 in reply to: ↑ 30 Changed 5 years ago by kcrisman

Or perhaps I am wrong all the way from the start and nobody cares about new functionality or history, so it absolutely does not matter what one can or cannot see in changelogs.

You are wrong if you believe that you can edit somebody else's ticket without asking for their opinion first. This is not a software development issue, this is called common sense.

To be far, Nathann, I think the intent was just to clarify the intent of the ticket to those who might not know the whole history of the 'useless classes'. This seems pretty reasonable, since there are hundreds (if not thousands) of people who browse Trac. Humor is a pretty slippery thing - all about context :-)

As to ownership, I would probably disagree that anyone "owns" any given ticket, any more than William "owns" Sage, but rather that there is significant moral authority vested in knowing the particular code and situation very well. So people shouldn't change titles without reason, but if there is a helpful title change (as this one was clearly intending to be) we can agree it's in the common good. Naturally, determining how the Sage community governs itself on this issue is for sage-devel - see also Steven Weber's "The Success of Open Source" for background reading on governance in open-source communities. Should really be required reading for anyone who spends more than a few hours a week on Trac :-)

comment:32 Changed 5 years ago by ncohen

  • Owner changed from (none) to ncohen

(I don't even know what this field is for, but I need to put my name there for political reasons)

comment:33 Changed 5 years ago by ncohen

Oh, by the way I answered the sage-devel thread : https://groups.google.com/forum/#!topic/sage-devel/do7EAtPq4Kg

Nathann

comment:34 Changed 5 years ago by ncohen

  • Summary changed from Useless new design classes and a replacement for _check_pbd to New empty design classes for a better user interface and new is_group_divisible_design Cython function

comment:35 Changed 5 years ago by git

  • Commit changed from 494d82657000af8d4361a630035227c6aab6f5de to e8251fb1c1f22a1b7c6a15bd8be9686cdd5f54e6

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

e8251fbtrac #16598: Merged with 6.3.beta6

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

  • Status changed from needs_review to needs_work

Hi Nathann,

1) There is a problem with your relabel method

sage: I = IncidenceStructure(3, [[0,1,2],[0,1]])
sage: I.relabel({0:'a',1:'b',2:'c'})
sage: I._point_to_index
{0: 0, 1: 1, 2: 2}

but should be {'a':0, 'b': 1, 'c': 2}.

2) For the string representation of divisible design, would not you prefer Group Divisible Design on 40 points of type 10^4 instead of the current Group Divisible Design on 40 points and type 10^4

3) This is very bad

round(sqrt(len(blocks)))

(i.e. it involves the symbolic ring). You would prefer using the method sqrtrem of Sage integers

sage: i,j = 103.sqrtrem()
sage: 103 == i**2 + j
True

4) In is_group_divisible_design the parts # Check that the groups belong to [0,...,n-1] and # Check that the groups belong to [0,...,n-1] are very complicated. It is not clear to me that it would be the fastest option (did you do some timings?). Would you mind having a more Pythonic version:

# Check that the groups belong to [0,...,n-1]
if any(i < 0 or i >= n for g in groups for i in g):
    # bad groups
    ...
if any(i < 0 or i >= n for b in blocks for i in b):
    # bad blocks
    ...

That's all for now.

Vincent

comment:37 in reply to: ↑ 36 ; follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

YOoooooooo !

1) There is a problem with your relabel method

Gloops. Sorry 'bout that :-/

2) For the string representation of divisible design, would not you prefer Group Divisible Design on 40 points of type 10^4 instead of the current Group Divisible Design on 40 points and type 10^4

I don't care. I changed it,

3) This is very bad

round(sqrt(len(blocks)))

(i.e. it involves the symbolic ring).

Is it bad to involve the symbolic ring ? O_o

I imported sqrt from Python instead.

4) In is_group_divisible_design the parts # Check that the groups belong to [0,...,n-1] and # Check that the groups belong to [0,...,n-1] are very complicated. It is not clear to me that it would be the fastest option (did you do some timings?). Would you mind having a more Pythonic version:

# Check that the groups belong to [0,...,n-1]
if any(i < 0 or i >= n for g in groups for i in g):
    # bad groups
    ...
if any(i < 0 or i >= n for b in blocks for i in b):
    # bad blocks
    ...

This code raises an exception when i cannot be turned into an integer. I simplified it a bit, though.

Nathann

comment:38 Changed 5 years ago by git

  • Commit changed from e8251fb1c1f22a1b7c6a15bd8be9686cdd5f54e6 to 8a44b83e01080869d82e96c265d73761e9a779b6

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

8a44b83trac #16598: Review

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

Hi,

Could we move _relabel_bibd as a method of BalancedIncompleteBlockDesign?

(not very important) Why did you put the classes PairwiseBalancedDesign and BalancedIncompleteBlockDesign in the file bibd.py and not in incidence_structure.py? I am not sure about what is the best organization should be, but right now it is not clear at all where to look to find something.

Vincent

comment:40 in reply to: ↑ 39 Changed 5 years ago by ncohen

Could we move _relabel_bibd as a method of BalancedIncompleteBlockDesign?

Hmmmm.. Well, it woud mean that we have to build BIBD objects during the recursive constructions too... I'd prefer to keep it this way if you don't mind :-/

(not very important) Why did you put the classes PairwiseBalancedDesign and BalancedIncompleteBlockDesign in the file bibd.py and not in incidence_structure.py? I am not sure about what is the best organization should be, but right now it is not clear at all where to look to find something.

Well, that BalancedIncompleteBlockDesign is defined is bibd.py is hardly a surprise. I feel that PBD are very close to BIBD but indeed, if GDD are in IncidenceStructure then indeed it can be moved there. What do you think ?

Nathann

comment:41 Changed 5 years ago by git

  • Commit changed from 8a44b83e01080869d82e96c265d73761e9a779b6 to ea8b144f5dd28d40102e5ccb06edab716951867a

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

ea8b144trac #16598: Code simplification

comment:42 in reply to: ↑ 37 ; follow-up: Changed 5 years ago by vdelecroix

Replying to ncohen:

YOoooooooo !

1) There is a problem with your relabel method

Gloops. Sorry 'bout that :-/

There is still a subtle issue. When we ask for a relabeling we do not necessarily want to relabel the points. More precisely, if the ground set is {0,1,2,3} and I want to relabel {0:1, 1:3, 2:1, 3:0} I would like the ground set to be fixed and the blocks to be changed... what do you think?

3) This is very bad

round(sqrt(len(blocks)))

(i.e. it involves the symbolic ring).

Is it bad to involve the symbolic ring ? O_o

Yes: SR is very slow and unreliable (though for sqrt of integers it should be ok).

Vincent

comment:43 Changed 5 years ago by vdelecroix

updated version at u/vdelecroix/16598 (above 6.3.rc1 and #16766)

Vincent

comment:44 in reply to: ↑ 42 ; follow-up: Changed 5 years ago by ncohen

Yoooooooo !!

There is still a subtle issue. When we ask for a relabeling we do not necessarily want to relabel the points. More precisely, if the ground set is {0,1,2,3} and I want to relabel {0:1, 1:3, 2:1, 3:0} I would like the ground set to be fixed and the blocks to be changed... what do you think?

I don't understand. You are just relabeling with the same ground sets, what's the problem with that ? And how does that badly affect the blocks ? Some will change, some will not, what's the problem ?

Yes: SR is very slow and unreliable (though for sqrt of integers it should be ok).

"Bôf". If it does not work it's a bug report, and if Sage cannot compute square roots safely we should fork it quick :-P

Nathann

comment:45 Changed 5 years ago by ncohen

  • Branch changed from u/ncohen/16598 to u/vdelecroix/16598
  • Commit changed from ea8b144f5dd28d40102e5ccb06edab716951867a to 1a9ae44eb163fd218e592c47ecb49a87cb7741d2

New commits:

081aa8btrac #16598: merge with 6.3.rc1
49ae09dtrac #16766: Improve the doc of combinat/designs/
d626290trac #16766: we don t want designs.deprecated_function_alias
8f9ee77trac #16766: Broken doctests
47cebb3trac #16766: Broken doctest
6cda4b6trac #16766: Git 101: How to create a conflict with 10 others patches in needs_review
06e330btrac #16766: form -> from
1a9ae44trac #16598: merge #16766 (documentation update)

comment:46 Changed 5 years ago by vdelecroix

  • Dependencies changed from #16553 to #16553, #16766

comment:47 in reply to: ↑ 44 ; follow-up: Changed 5 years ago by vdelecroix

Replying to ncohen:

Yoooooooo !!

There is still a subtle issue. When we ask for a relabeling we do not necessarily want to relabel the points. More precisely, if the ground set is {0,1,2,3} and I want to relabel {0:1, 1:3, 2:1, 3:0} I would like the ground set to be fixed and the blocks to be changed... what do you think?

I don't understand. You are just relabeling with the same ground sets, what's the problem with that ? And how does that badly affect the blocks ? Some will change, some will not, what's the problem ?

sage: I = IncidenceStructure(4,[[0,1,2],[1,3]])
sage: I.ground_set()
[0, 1, 2, 3]
sage: I.relabel({0:3,1:2,2:1,3:0})
sage: I.ground_set()
[3, 2, 1, 0]

the ground set has changed...

Vincent

comment:48 in reply to: ↑ 47 Changed 5 years ago by ncohen

Yooooo !

sage: I = IncidenceStructure(4,[[0,1,2],[1,3]])
sage: I.ground_set()
[0, 1, 2, 3]
sage: I.relabel({0:3,1:2,2:1,3:0})
sage: I.ground_set()
[3, 2, 1, 0]

the ground set has changed...

Well, as a list it has, not as a set...

I think that being smart here only raises new problems. Relabelling all blocks takes much more time and memory than relabelling the points only. We could hide that by returning a sorted list of points, but well...

Nathann

comment:49 Changed 5 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Hello,

All right, we will see in the future.

Vincent

comment:50 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:51 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:52 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_work to positive_review

comment:53 Changed 5 years ago by ncohen

  • Branch changed from u/vdelecroix/16598 to public/16598
  • Commit changed from 1a9ae44eb163fd218e592c47ecb49a87cb7741d2 to 5f1fb98b5a72459b220b87f1c902bda62e4a166a

New commits:

5f1fb98trac #16598: Merged with 6.3

comment:54 Changed 5 years ago by vbraun

  • Branch changed from public/16598 to 5f1fb98b5a72459b220b87f1c902bda62e4a166a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.