#16362: Orthogonal Polar Graph
-------------------------------------+-------------------------------------
Reporter: ncohen | Owner:
Type: enhancement | Status: positive_review
Priority: major | Milestone: sage-6.3
Component: graph theory | Resolution:
Keywords: | Merged in:
Authors: Nathann Cohen | Reviewers: Dima Pasechnik
Report Upstream: N/A | Work issues:
Branch: | Commit:
public/ticket/16362 | ff7d3387b4436411ed6785e2a3b6c5c37f2ca1f6
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by dimpase):
Replying to [comment:45 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`?
--
Ticket URL: <http://trac.sagemath.org/ticket/16362#comment:47>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
