Opened 6 years ago
Closed 6 years ago
#16362 closed enhancement (fixed)
Orthogonal Polar Graph
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.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 )
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 6 years ago by
 Branch set to u/ncohen/16362
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit set to 6adc14bfb2949e2b9c79d82f53bf2f70939e5725
comment:3 Changed 6 years ago by
 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:
f1e4fb3  just a small pep8 cleanup

comment:4 Changed 6 years ago by
 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 6 years ago by
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?!
comment:6 Changed 6 years ago by
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 followup: ↓ 12 Changed 6 years ago by
 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 6 years ago by
 Commit changed from f1e4fb3030235b2908b2c1d1355413673aaf75cc to f45989e2e2e75488ac6a9bd68abd1c3503d4e8f8
Branch pushed to git repo; I updated commit sha1. New commits:
f45989e  trac #16362: Reviewer's comments

comment:9 Changed 6 years ago by
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/sitepackages/sage/structure/element.so in sage.structure.element.Vector.__mul__ (sage/structure/element.c:19059)() /home/ncohen/.Sage/local/lib/python2.7/sitepackages/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/sitepackages/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/sitepackages/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 6 years ago by
 Commit changed from f45989e2e2e75488ac6a9bd68abd1c3503d4e8f8 to e3c4aaa24853f56dccd947c250aea8834b3b1ea6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e3c4aaa  trac #16362: Reviewer's comments

comment:11 Changed 6 years ago by
Okay, fixed :P
Nathann
comment:12 in reply to: ↑ 7 ; followup: ↓ 13 Changed 6 years ago by
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 2^{k
}
with k>1. This case behaves differently from the odd characteristic case...
New commits:
e3c4aaa  trac #16362: Reviewer's comments

comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 6 years ago by
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 2^{k }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 ; followup: ↓ 15 Changed 6 years ago by
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 2^{k }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 6 years ago by
#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 6 years ago by
 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/sitepackages/sage/graphs/graph_generators.py:docstring of sage.graphs.graph_generators.GraphGenerators.OrthogonalPolarGraph:10: WARNING: Inline literal startstring without endstring.
comment:17 Changed 6 years ago by
 Commit changed from e3c4aaa24853f56dccd947c250aea8834b3b1ea6 to 0089d6c9962ac3002d3aa3b1a9f88e996f784b6e
Branch pushed to git repo; I updated commit sha1. New commits:
0089d6c  trac #16362: Broken documentation

comment:19 Changed 6 years ago by
 Commit changed from 0089d6c9962ac3002d3aa3b1a9f88e996f784b6e to 272e18fe75c02a3dc38b6f5e2518d5988c4350b4
Branch pushed to git repo; I updated commit sha1. New commits:
272e18f  trac #16362: Broken doc 2

comment:20 Changed 6 years ago by
 Branch changed from public/ticket/16362 to u/ncohen/16362
comment:21 Changed 6 years ago by
I'd like to libGAPify
this and #14631. That gap("....
stuff sucks. Patches coming.
comment:22 Changed 6 years ago by
 Branch changed from u/ncohen/16362 to u/dimpase/16362
comment:23 followup: ↓ 24 Changed 6 years ago by
 Commit changed from 272e18fe75c02a3dc38b6f5e2518d5988c4350b4 to bcdbf21d0c5033696e33b6ec6b31cda3940013c1
if you're happy with my changes then please set it to positive review...
New commits:
a9faba0  trac #14631: Affine Polar Graphs

a1ec9b5  trac #14631: Merged with 6.3.beta1

c5cc1d9  trac #14631: Merged with #16362

e725be9  trac #14631: Note added

257082d  trac #14631: Reviewer's comments

4f82fa5  trac #14631: Merged with #16362

3ef564e  libGAPification

bcdbf21  libGAPisation and some cleanup

comment:24 in reply to: ↑ 23 Changed 6 years ago by
comment:25 followup: ↓ 26 Changed 6 years ago by
 Dependencies set to #14631
comment:26 in reply to: ↑ 25 Changed 6 years ago by
 Dependencies #14631 deleted
Come on Dima, do a clean job please !
Nathann
comment:27 Changed 6 years ago by
 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 6 years ago by
 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 cherrypick (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 6 years ago by
 Branch changed from u/dimpase/16362 to public/ticket/16362
 Commit changed from bcdbf21d0c5033696e33b6ec6b31cda3940013c1 to ff7d3387b4436411ed6785e2a3b6c5c37f2ca1f6
comment:30 Changed 6 years ago by
like this?
comment:31 Changed 6 years ago by
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 followup: ↓ 33 Changed 6 years ago by
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 ; followup: ↓ 34 Changed 6 years ago by
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 6 years ago by
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 followup: ↓ 36 Changed 6 years ago by
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 ; followup: ↓ 37 Changed 6 years ago by
comment:37 in reply to: ↑ 36 Changed 6 years ago by
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 6 years ago by
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 followup: ↓ 40 Changed 6 years ago by
Dima, it takes +15 seconds to test the families.py file for absolutely NOTHING.
Nathann
comment:40 in reply to: ↑ 39 ; followup: ↓ 41 Changed 6 years ago by
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 6 years ago by
 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 followup: ↓ 43 Changed 6 years ago by
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 ; followup: ↓ 44 Changed 6 years ago by
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 ; followup: ↓ 45 Changed 6 years ago by
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 sagedevelop? 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 ; followup: ↓ 47 Changed 6 years ago by
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
comment:47 in reply to: ↑ 45 ; followup: ↓ 48 Changed 6 years ago by
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 sagedevel.
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 ; followup: ↓ 52 Changed 6 years ago by
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 sagedevel.
You keep mentionning this as if you thought that it might be a problem for me. If you have something to bring up on sagedevel 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 topositive_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 followup: ↓ 50 Changed 6 years ago by
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
comment:50 in reply to: ↑ 49 Changed 6 years ago by
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 followup: ↓ 53 Changed 6 years ago by
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 ; followups: ↓ 54 ↓ 57 Changed 6 years ago by
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 sagedevel.
You keep mentionning this as if you thought that it might be a problem for me. If you have something to bring up on sagedevel 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 ; followup: ↓ 55 Changed 6 years ago by
comment:54 in reply to: ↑ 52 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
By the way the 20 of July is a sunday. Lucky you.
Nathann
comment:57 in reply to: ↑ 52 ; followup: ↓ 58 Changed 6 years ago by
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 ; followup: ↓ 59 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
 Branch changed from public/ticket/16362 to ff7d3387b4436411ed6785e2a3b6c5c37f2ca1f6
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #16362: Polar Graph