Opened 5 years ago

Closed 5 years ago

#16362 closed enhancement (fixed)

Orthogonal Polar Graph

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.3
Component: graph theory Keywords:
Cc: dimpase Merged in:
Authors: Nathann Cohen Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: ff7d338 (Commits) Commit: ff7d3387b4436411ed6785e2a3b6c5c37f2ca1f6
Dependencies: Stopgaps:

Description (last modified by ncohen)

Orthogonal Polar Graphs ! Brand new O^+(m,q), O^-(m,q) and O(m,q) strongly regular graphs from Brouwer's website ! :-P

Nathann

Change History (60)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16362
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to 6adc14bfb2949e2b9c79d82f53bf2f70939e5725

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

6adc14btrac #16362: Polar Graph

comment:3 Changed 5 years ago by chapoton

  • Branch changed from u/ncohen/16362 to public/ticket/16362
  • Commit changed from 6adc14bfb2949e2b9c79d82f53bf2f70939e5725 to f1e4fb3030235b2908b2c1d1355413673aaf75cc

not a review, just done a little work to turn code in pep8 standard


New commits:

f1e4fb3just a small pep8 cleanup

comment:4 Changed 5 years ago by dimpase

  • Status changed from needs_review to needs_work

There is no canonical way to define GF(q). You somehow should synchronise the GF(q) used in your construction with the GF(q) used by GAP in your call to get the quadratic form. IIRC if you use the parameter conway=True in Sage then it will be in sync with GAP, which uses Conway polynomials to construct GF(q). Or you can pass the GF(q) to GAP (or the other way around).

comment:5 Changed 5 years ago by dimpase

I'd also like to point out that the way F(x,y) in the code is computed is rather weird and hard to read. A much more straightforward implementation would just be

F=lambda x,y: vector(x)*M*vector(y)

for M the matrix of the form; i.e. do not convert M into a dictionary.

(ADDED): you never call F(x,y) with x not equal to y, why do you need two arguments?!

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

comment:6 Changed 5 years ago by dimpase

The name of the function has to be changed. Classically, polar graphs (i.e. the collineartity graphs of polar spaces) are these ones, the symplectic ones, and the unitary ones. You have a Hermitean, or symplectic (or quadratic) form F, and the vertices of the graphs are the isotropic (resp. singular)) w.r.t to F.

We already have the symplectic case, this ticket covers the quadratic (a.k.a. orthogonal) case, it remains to cover the Hermitean case (called U(n,q) in Brouwer's tables).

Let us call the graphs here OrthogonalPolarGraphs or something like this, to distinguish them from the rest.

comment:7 follow-up: Changed 5 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from Polar Graph to Orthogonal Polar Graph

What about this ? :-)

Nathann

comment:8 Changed 5 years ago by git

  • Commit changed from f1e4fb3030235b2908b2c1d1355413673aaf75cc to f45989e2e2e75488ac6a9bd68abd1c3503d4e8f8

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

f45989etrac #16362: Reviewer's comments

comment:9 Changed 5 years ago by ncohen

Oh, and about the multiplication : I was no able to make it work, because of error message I do not understand.

   2247     def F(x):
-> 2248         return x*M*x
   2249 
   2250     def FF(x,y):

/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Vector.__mul__ (sage/structure/element.c:19059)()

/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:8274)()

TypeError: 'sage.modules.vector_modn_dense.Vector_modn_dense' object cannot be interpreted as an index

Or even

/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/graphs/generators/families.pyc in F(x)
   2246 
   2247     def F(x):
-> 2248         return x*M*x.column()
   2249 
   2250     def FF(x,y):

/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/schemes/generic/morphism.pyc in __mul__(self, right)
    378         """
    379         if not isinstance(right, SchemeMorphism):
--> 380             raise TypeError, "right (=%s) must be a SchemeMorphism to multiply it by %s"%(right, self)
    381         if right.codomain() != self.domain():
    382             raise TypeError, "self (=%s) domain must equal right (=%s) codomain"%(self, right)

TypeError: right (=[((0, 1), 1), ((2, 2), 1), ((4, 4), 1), ((5, 5), 1), ((3, 3), 1)]) must be a SchemeMorphism to multiply it by (0 : 0 : 0 : 0 : 0 : 1)

And I am tired to fight with the category bugs that have been left unfixed for years.

If you can make it work I don't mind, otherwise I will chose a good night's sleep over this debugging hell :-P

Nathann

comment:10 Changed 5 years ago by git

  • Commit changed from f45989e2e2e75488ac6a9bd68abd1c3503d4e8f8 to e3c4aaa24853f56dccd947c250aea8834b3b1ea6

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

e3c4aaatrac #16362: Reviewer's comments

comment:11 Changed 5 years ago by ncohen

Okay, fixed :-P

Nathann

comment:12 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

What about this ? :-)

you don't believe in linear algebra, do you? :p

How about

def F(x):
   return x*M*x

after replacing M = Matrix(M).dict().items() with M = Matrix(M) ?

And I would also added some tests for Fq being of order 2k

with k>1. This case behaves differently from the odd characteristic case...


New commits:

e3c4aaatrac #16362: Reviewer's comments

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

you don't believe in linear algebra, do you? :p

I don't believe in anything that is implemented in Sage and involves Parents, Elements, or categories.

And I would also added some tests for Fq being of order 2k

with k>1. This case behaves differently from the odd characteristic case...

Could you add a commit for that, please ?

Nathann

P.S. : If you feel like reviewing stuff, #14631 is 1 year old :-P

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

Replying to ncohen:

you don't believe in linear algebra, do you? :p

I don't believe in anything that is implemented in Sage and involves Parents, Elements, or categories.

And I would also added some tests for Fq being of order 2k

with k>1. This case behaves differently from the odd characteristic case...

Could you add a commit for that, please ?

Nathann

P.S. : If you feel like reviewing stuff, #14631 is 1 year old :-P

#14631 does use x*M*x. Please be consistent in your coding... :P

comment:15 in reply to: ↑ 14 Changed 5 years ago by ncohen

#14631 does use x*M*x. Please be consistent in your coding... :P

Come on Dima. You know how much I hate consistency.

Nathann

comment:16 Changed 5 years ago by rws

  • Status changed from needs_review to needs_work
Error building the documentation.
Traceback (most recent call last):
  File "/home/ralf/sage/src/doc/common/builder.py", line 1477, in <module>
    getattr(get_builder(name), type)()
  File "/home/ralf/sage/src/doc/common/builder.py", line 276, in _wrapper
    getattr(get_builder(document), 'inventory')(*args, **kwds)
  File "/home/ralf/sage/src/doc/common/builder.py", line 487, in _wrapper
    x.get(99999)
  File "/home/ralf/sage/local/lib/python/multiprocessing/pool.py", line 554, in get
    raise self._value
OSError: [graphs   ] /home/ralf/sage/local/lib/python2.7/site-packages/sage/graphs/graph_generators.py:docstring of sage.graphs.graph_generators.GraphGenerators.OrthogonalPolarGraph:10: WARNING: Inline literal start-string without end-string.

comment:17 Changed 5 years ago by git

  • Commit changed from e3c4aaa24853f56dccd947c250aea8834b3b1ea6 to 0089d6c9962ac3002d3aa3b1a9f88e996f784b6e

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

0089d6ctrac #16362: Broken documentation

comment:18 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

Fixed !

comment:19 Changed 5 years ago by git

  • Commit changed from 0089d6c9962ac3002d3aa3b1a9f88e996f784b6e to 272e18fe75c02a3dc38b6f5e2518d5988c4350b4

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

272e18ftrac #16362: Broken doc 2

comment:20 Changed 5 years ago by ncohen

  • Branch changed from public/ticket/16362 to u/ncohen/16362

comment:21 Changed 5 years ago by dimpase

I'd like to libGAPify this and #14631. That gap(".... stuff sucks. Patches coming.

comment:22 Changed 5 years ago by dimpase

  • Branch changed from u/ncohen/16362 to u/dimpase/16362

comment:23 follow-up: Changed 5 years ago by dimpase

  • Commit changed from 272e18fe75c02a3dc38b6f5e2518d5988c4350b4 to bcdbf21d0c5033696e33b6ec6b31cda3940013c1

if you're happy with my changes then please set it to positive review...


New commits:

a9faba0trac #14631: Affine Polar Graphs
a1ec9b5trac #14631: Merged with 6.3.beta1
c5cc1d9trac #14631: Merged with #16362
e725be9trac #14631: Note added
257082dtrac #14631: Reviewer's comments
4f82fa5trac #14631: Merged with #16362
3ef564elibGAPification
bcdbf21libGAPisation and some cleanup

comment:24 in reply to: ↑ 23 Changed 5 years ago by ncohen

if you're happy with my changes then please set it to positive review...

Well, not really... Now you have in the branch of #16362 some commits that belong to #14631, while #14631 depends on #16362 ....

Nathann

comment:25 follow-up: Changed 5 years ago by dimpase

  • Dependencies set to #14631

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

  • Dependencies #14631 deleted

Come on Dima, do a clean job please !

Nathann

comment:27 Changed 5 years ago by dimpase

  • Dependencies set to #14631

Obviously, I only meant to add ​bcdbf21: libGAPisation and some cleanup, not the whole slew of things. If you know how to untangle this mess please advise.

comment:28 Changed 5 years ago by ncohen

  • Dependencies #14631 deleted

No Dima, this ticket does NOT depend on the other, it is the other way around.

You worked on the wrong branch. So now what you should do is checkout the original version of this branch, i.e. u/ncohen/16362, and cherry-pick (git sland for copy) the commits that you added on the wrong branch into where you are.

When this is done, push this new branch (with a -f, because you will be rewriting history) on u/dimpase/16362.

Nathann

comment:29 Changed 5 years ago by dimpase

  • Branch changed from u/dimpase/16362 to public/ticket/16362
  • Commit changed from bcdbf21d0c5033696e33b6ec6b31cda3940013c1 to ff7d3387b4436411ed6785e2a3b6c5c37f2ca1f6

New commits:

6adc14btrac #16362: Polar Graph
f1e4fb3just a small pep8 cleanup
e3c4aaatrac #16362: Reviewer's comments
0089d6ctrac #16362: Broken documentation
272e18ftrac #16362: Broken doc 2
ff7d338libGAPify and cleanup

comment:30 Changed 5 years ago by dimpase

like this?

comment:31 Changed 5 years ago by ncohen

Yes.

But now, there is this problem :

My branch:

sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 2.04 s, sys: 32 ms, total: 2.07 s
Wall time: 2.25 s

Yours:

sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 8.98 s, sys: 88 ms, total: 9.06 s
Wall time: 9.09 s

Nathann

comment:32 follow-up: Changed 5 years ago by dimpase

No, it's not a problem. This is just initialisation that takes longer due to lazy import of libgap. If I repeat tests on my branch I get

sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 3.58 s, sys: 61 ms, total: 3.64 s
Wall time: 3.65 s
sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 735 ms, sys: 12 ms, total: 747 ms
Wall time: 728 ms
sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 757 ms, sys: 6 ms, total: 763 ms
Wall time: 757 ms

and on your branch I get

sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 795 ms, sys: 17 ms, total: 812 ms
Wall time: 948 ms
sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 744 ms, sys: 2 ms, total: 746 ms
Wall time: 755 ms
sage: %time G=graphs.OrthogonalPolarGraph(6,3,"+"); G
CPU times: user 761 ms, sys: 4 ms, total: 765 ms
Wall time: 777 ms

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

No, it's not a problem.

If a code goes from 2s to 9s, it is a problem.

This is just initialisation that takes longer due to lazy import of libgap. If I repeat tests on my branch I get

Okay. Give me one reason to use libgap then.

Nathann

comment:34 in reply to: ↑ 33 Changed 5 years ago by dimpase

Replying to ncohen:

No, it's not a problem.

If a code goes from 2s to 9s, it is a problem.

if you lazify gap import, you'll see the same picture...

This is just initialisation that takes longer due to lazy import of libgap. If I repeat tests on my branch I get

Okay. Give me one reason to use libgap then.

here it's just the code that is cleaner. In general, it speeds up things quite a bit.

comment:35 follow-up: Changed 5 years ago by ncohen

Okay. Now, here, it is clearly slower the first time, and totally equivalent afterwards.

Please remove this part of your commit.

Nathann

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

Replying to ncohen:

Okay. Now, here, it is clearly slower the first time, and totally equivalent afterwards.

Please remove this part of your commit.

What part?

Besides, it's the same design as in #14631 which you happily approved...

comment:37 in reply to: ↑ 36 Changed 5 years ago by ncohen

What part?

Well, the call to libgap obviously.

Besides, it's the same design as in #14631 which you happily approved...

Well, I had not noticed that your improvements made the code slower. Please remove it from there too.

Nathann

comment:38 Changed 5 years ago by dimpase

No. This is not called "slower". If a call to libGAP happens before, it will not be slower. Besides, the pexpect interface (the one your code uses) is known to be very flaky, and is known to randomly stall or get extremely slow on certain Linux kernels (e.g. it was the case with Debian a while ago). LibGAP is much more robust in this respect.

As well, at some point libGAP will become the main interface to GAP, and all of a sudden the code using the old interface will get slower.

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

Dima, it takes +15 seconds to test the families.py file for absolutely NOTHING.

Nathann

comment:40 in reply to: ↑ 39 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

Dima, it takes +15 seconds to test the families.py file for absolutely NOTHING.

Robustness of the code is worth 15 seconds. Get a life!

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

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

Robustness of the code is worth 15 seconds. Get a life!

Same to you.

Okay, now positive review to this patch because nobody but you will ever review it otherwise (which is why I am stuck), and I will remove this "thing" in another branch.

Thank you for your help,

Nathann

comment:42 follow-up: Changed 5 years ago by dimpase

I don't understand your position. If you want to sneak your worse code through the back door, I will protest.

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

I don't understand your position. If you want to sneak your worse code through the back door, I will protest.

It is very simple : I work with this code, I need it to be fast. If a small change makes it go faster, I do it.

You would be surprised to see how many people also think that it makes sense.

Nathann

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

Replying to ncohen:

I don't understand your position. If you want to sneak your worse code through the back door, I will protest.

It is very simple : I work with this code, I need it to be fast. If a small change makes it go faster, I do it.

The code is as fast in my version as it is in yours. Your measurements are not correctly done. Do I need to explain why, again? Or do I need to raise it up on sage-develop? We can do the latter, sure.

The disadvantage of your code is that it is entirely possible that the pexpect interface gets (half)broken yet again.

Do I need to explain this? Or you just won't listen, because you think you know better? As well, let me point out that I find your suggestion that you will get rid of my changes in some way that I won't notice very, very worrying. This is not the way things are meant to be done.

And, finally, the design of the these tickets is very suboptimal, anyway, as they should be done with a backend that can take advantage of rich symmetries of the objects. I think I explained this on more than one occasion. GAP's GRAPE package can easily deal with graphs from this family with 10 times as many vertices as your code here. That's why I find your worry about a couple of more seconds on startup just funny...

You would be surprised to see how many people also think that it makes sense.

You might be surprised to find how many people find your outburst like this one very immature.

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

The code is as fast in my version as it is in yours.

Of course not. Your code has a penalty on first run, mine does not.

(just for the joke: do you also profile cached functions with %timeit ?)

The disadvantage of your code is that it is entirely possible that the pexpect interface gets (half)broken yet again.

1) It is not broken (nor even deprecated), until proven otherwise

2) A LOT of code in Sage uses it. If you consider this a bug, write a ticket to address that.

As well, let me point out that I find your suggestion that you will get rid of my changes in some way that I won't notice very, very worrying. This is not the way things are meant to be done.

You see it wrong again.

Right now, you are trying to force a commit (libgapify) into Sage even though I do not agree with it (me, the reviewer of your changes). This, indeed, is not the way things are meant to be done. Since when do people add stuff in Sage without getting the agreement of somebody else ? This is precisely what the review process is.

As you refuse discussion on the subject, I can either give up the code of a useful ticket or give in to your blackmail (your libgapify commit Vs my graph constructor) and say that I give your code a positive review.

Admittedly, I prefer this version of the code to no constructor at all.

Secondly, it is fortunate that mistakes in Sage can be fixed, and if another reviewer thinks that it is stupid to lose time for no gain then this will be undone. The review process.

Note how I will have to get the reviewer's agreement while you are just imposing your own view here, when I repeatedly asked to you to remove it and without anybody else's agreement for your commit.

This, clearly, is abnormal.

And, finally, the design of the these tickets is very suboptimal, anyway, as they should be done with a backend that can take advantage of rich symmetries of the objects. I think I explained this on more than one occasion.

I agree with you, but this backend does not exist in Sage. Did you miss this detail ?

I don't even know how to code such a thing. So if this is a problem for you, stop raving and implement it. I would be glad to have it too.

You might be surprised to find how many people find your outburst like this one very immature.

The number of people who think that I am immature has no bounds. I definitely remember that I used to care, a long time ago. Fortunately, with time I figured out that they were all crazy, which simplified a lot of things.

Nathann

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

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

Replying to ncohen:

The code is as fast in my version as it is in yours.

Of course not. Your code has a penalty on first run, mine does not.

No, you are simply misled by %time here. Your startup times only *look* faster, for purely historical reasons, as an instance of GAP on pexpect interface gets loaded and GAP spawned prior to the 1st function call, and %time does not show this extra overhead, but an instance of libGAP gets loaded at the 1st function call.

Anyhow, thanks for rising the point startup time of libGAP. Hopefully it can be improved even if it is still lazily imported.

The disadvantage of your code is that it is entirely possible that the pexpect interface gets (half)broken yet again.

1) It is not broken (nor even deprecated), until proven otherwise

Let me assure you that you do not want to know the details, really; pexpect is known to be flaky, abandoned code which might break with on newer Linux (or other OSes) again, just as it did in the past, as was extensively discussed, worked around (Linux kernel patches were necessary). Using libraries instead of it is a clear way out of this flakiness.

2) A LOT of code in Sage uses it. If you consider this a bug, write a ticket to address that.

As well, let me point out that I find your suggestion that you will get rid of my changes in some way that I won't notice very, very worrying. This is not the way things are meant to be done.

You see it wrong again.

Right now, you are trying to force a commit (libgapify) into Sage even though I do not agree with it (me, the reviewer of your changes).

I am not forcing anything. Certainly, you can convince me to change it, or change it yourself. I gave you a number of arguments (readability of the code, robustness) why there is an advantage to use libGAP. You dismissed them all without any reasons, except that "slowdown".

I may give you one more argument: my code makes sure that the finite fields used by GAP and by Sage are the same (mathematically). Your code does not really address this. Your code relies on an assumption, that currently appears to be true (well, at least I think so) that GAP and Sage use the same algorithm, based on Conway polynomials.

This, indeed, is not the way things are meant to be done. Since when do people add stuff in Sage without getting the agreement of somebody else ? This is precisely what the review process is.

What you appear to have suggested that you accept my code only to change it later, in another branch, with another reviewer. And I found this very worrying, as this undermines the way Sage reviews work.

As you refuse discussion on the subject, I can either give up the code of a useful ticket or give in to your blackmail (your libgapify commit Vs my graph constructor) and say that I give your code a positive review.

Admittedly, I prefer this version of the code to no constructor at all.

Secondly, it is fortunate that mistakes in Sage can be fixed, and if another reviewer thinks that it is stupid to lose time for no gain then this will be undone. The review process.

Note how I will have to get the reviewer's agreement while you are just imposing your own view here, when I repeatedly asked to you to remove it and without anybody else's agreement for your commit.

You did not ask me to remove my commit. You asked me to change a part of the code that relies on libGAP, didn't you? Without listening to any of the arguments I gave you in its favour.

This, clearly, is abnormal.

Indeed, what is happening on this ticket is very abnormal, and you might force me to draw attention to it on sage-devel.

And, finally, the design of the these tickets is very suboptimal, anyway, as they should be done with a backend that can take advantage of rich symmetries of the objects. I think I explained this on more than one occasion.

I agree with you, but this backend does not exist in Sage. Did you miss this detail ?

I don't even know how to code such a thing. So if this is a problem for you, stop raving and implement it. I would be glad to have it too.

I merely mentioned this to draw your attention to the fact that arguing about the startup speed of something which is at best a prototype implementation is silly.

You might be surprised to find how many people find your outburst like this one very immature.

The number of people who think that I am immature has no bounds. I definitely remember that I used to care, a long time ago. Fortunately, with time I figured out that they were all crazy, which simplified a lot of things.

Well, craziness is relative, FYI. Craziness is often an just a socially unacceptable form of behaviour. Think about it :-)

Should we set the status of this ticket back to needs_review, as you apparently admit yourself that you do not act in good faith when you switched it to positive_review?

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

Yooooooooo !!

No, you are simply misled by %time here.

And by sage -t too.

I am not forcing anything. Certainly, you can convince me to change it, or change it yourself. I gave you a number of arguments (readability of the code, robustness) why there is an advantage to use libGAP. You dismissed them all without any reasons, except that "slowdown".

Well, yes. This is a reason that is reminded to me every time I start Sage and create those strongly regular graphs. It is a down side for me, a user of that code.

I may give you one more argument: my code makes sure that the finite fields used by GAP and by Sage are the same (mathematically). Your code does not really address this. Your code relies on an assumption, that currently appears to be true (well, at least I think so) that GAP and Sage use the same algorithm, based on Conway polynomials.

You did not mention this before. Do you think that a lot of code in Sage is based on this assumption ?

What you appear to have suggested that you accept my code only to change it later, in another branch, with another reviewer. And I found this very worrying, as this undermines the way Sage reviews work.

Indeed. Be worried of what may happen with the Graph code in Sage, it is clearly unsafe when I am around.

You did not ask me to remove my commit. You asked me to change a part of the code that relies on libGAP, didn't you?

Indeed, I asked you to remove that part. Do you really believe that all this discussion is about the typos that you fixed ?

Indeed, what is happening on this ticket is very abnormal, and you might force me to draw attention to it on sage-devel.

You keep mentionning this as if you thought that it might be a problem for me. If you have something to bring up on sage-devel send an email. I am not doing anything secret here that I would not want to be known.

When I rape and commit murder, I usually don't mention it on public tickets.

I mean. There were exceptions, of course...

I merely mentioned this to draw your attention to the fact that arguing about the startup speed of something which is at best a prototype implementation is silly.

In what world do you live ? I *do not know* how to implement this, and if I did it would still be a massive amouont of work that I have no reason to do, because it is totally useless to me. Telling me to use a nonexistent backend that I don't know how to write does not lead anywhere. Implement it, and of course this code will use it.

Should we set the status of this ticket back to needs_review, as you apparently admit yourself that you do not act in good faith when you switched it to positive_review?

Well, I think that this ticket is a good addition, even though you made the function slower on the first run for bad reasons. Not a sufficient reason to hold it back, even though it is a pity, of course.

Nathann

comment:49 follow-up: Changed 5 years ago by ncohen

By the way : the correct procedure to settle this is the following :

1) Implement this code with the current gap interface, because it is faster and not deprecated, and because 2) and 3) may take forever.

2) Improve libgap

3) Deprecate other calls to gap (which means updating this code too)

Nathann

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

comment:50 in reply to: ↑ 49 Changed 5 years ago by dimpase

Replying to ncohen:

2) Improve libgap

let me see how far 2) can be pushed. If this is indeed a hard one, I promise I will switch this code back to classical GAP interface, say, by November this year (please feel free to remind me then).

comment:51 follow-up: Changed 5 years ago by ncohen

SIX MONTHS ?

Change this to two months and it is a deal. I don't put much faith in people telling me "I will do it later". I have #14019 and Florent's "done by next month" 16 months ago.

If you honestly believe that you will do it, you can also accept to use the classical GAP interface and work on 2). When it is done, I see no problem with updating this code.

See ? It's clearly better this way.

Nathann

comment:52 in reply to: ↑ 48 ; follow-ups: Changed 5 years ago by dimpase

Replying to ncohen:

Yooooooooo !!

No, you are simply misled by %time here.

And by sage -t too.

I am not forcing anything. Certainly, you can convince me to change it, or change it yourself. I gave you a number of arguments (readability of the code, robustness) why there is an advantage to use libGAP. You dismissed them all without any reasons, except that "slowdown".

Well, yes. This is a reason that is reminded to me every time I start Sage and create those strongly regular graphs. It is a down side for me, a user of that code.

I may give you one more argument: my code makes sure that the finite fields used by GAP and by Sage are the same (mathematically). Your code does not really address this. Your code relies on an assumption, that currently appears to be true (well, at least I think so) that GAP and Sage use the same algorithm, based on Conway polynomials.

You did not mention this before.

I did mention this in comment 4 above.

Do you think that a lot of code in Sage is based on this assumption ?

I am not sure. There are ways to make sure that the same field is used in GAP and in Sage with the classical interface. E.g. get the GAP's defining polynomial:

sage: gap.GF(4).DefiningPolynomial()
x_1^2+x_1+Z(2)^0

and use it to create Sage's field.

What you appear to have suggested that you accept my code only to change it later, in another branch, with another reviewer. And I found this very worrying, as this undermines the way Sage reviews work.

Indeed. Be worried of what may happen with the Graph code in Sage, it is clearly unsafe when I am around.

You did not ask me to remove my commit. You asked me to change a part of the code that relies on libGAP, didn't you?

Indeed, I asked you to remove that part. Do you really believe that all this discussion is about the typos that you fixed ?

Indeed, what is happening on this ticket is very abnormal, and you might force me to draw attention to it on sage-devel.

You keep mentionning this as if you thought that it might be a problem for me. If you have something to bring up on sage-devel send an email. I am not doing anything secret here that I would not want to be known.

When I rape and commit murder, I usually don't mention it on public tickets.

I mean. There were exceptions, of course...

This discussion DID cost me a couple of hours of sleep, FYI :-)

I merely mentioned this to draw your attention to the fact that arguing about the startup speed of something which is at best a prototype implementation is silly.

In what world do you live ? I *do not know* how to implement this, and if I did it would still be a massive amount of work that I have no reason to do, because it is totally useless to me.

Useless? You would hate to be able to work with polar graphs on 10000 vertices?

comment:53 in reply to: ↑ 51 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

SIX MONTHS ?

Change this to two months and it is a deal.

OK, make it two.

comment:54 in reply to: ↑ 52 Changed 5 years ago by ncohen

I did mention this in comment 4 above.

You told me in this comment to add conway=True, which I did. And now you tell me that this may not be correct either.

This discussion DID cost me a couple of hours of sleep, FYI :-)

Same here, but I cannot blame you for giving your point of view.

Useless? You would hate to be able to work with polar graphs on 10000 vertices?

I don't need it. Do you ? If you do, write a patch. Don't try to make me implement it for you. Again, I don't even know how to do that anyway.

Nathann

comment:55 in reply to: ↑ 53 Changed 5 years ago by ncohen

OK, make it two.

Deal. Today is the 20 of may. On the 20 of July I will send you an email :

1) Either you have a ticket that makes both code equivalent (no startup cost) in needs_review and it is a very nice outcome

2) Otherwise, you have in needs_review (that I will review) to undo those changes.

Also : don't tell me two months later that you were on holidays in July. You know it now, you know what to expect.

Nathann

comment:56 Changed 5 years ago by ncohen

By the way the 20 of July is a sunday. Lucky you.

Nathann

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

Useless? You would hate to be able to work with polar graphs on 10000 vertices?

By the way : I solve domination problems on those graphs. When their size is above 500, I give up.

Nathann

comment:58 in reply to: ↑ 57 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

Useless? You would hate to be able to work with polar graphs on 10000 vertices?

By the way : I solve domination problems on those graphs.

Are the best known solutions independent sets? There are bounds on the latter I should know a lot about...

comment:59 in reply to: ↑ 58 Changed 5 years ago by ncohen

Are the best known solutions independent sets? There are bounds on the latter I should know a lot about...

No, sorry. Well, it is not exactly domination problems but identifying codes (which are sometimes required to be dominating too). My colleague tells me that some identifying code are sometimes independent, but that it is very far from the actual bounds usually ^^;

Thanks, though !

Nathann

comment:60 Changed 5 years ago by vbraun

  • Branch changed from public/ticket/16362 to ff7d3387b4436411ed6785e2a3b6c5c37f2ca1f6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.