Implement center of a HyperplaneArrangement

[nailuj]

This ticket implements the following changes to the class HyperplaneArrangementElement:

• In the method is_central, add the argument certificate=False. If set to True, this will additionally return the center of the arrangement. The center is easily obtainable as a byproduct of the existing calculation.
• Add the method center(). This will return the center of the arrangement as computed by is_central(certificate=True).

[git]

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

​d8b282c

updated docstring of is_central

[jipilab]

Here are few schönheitssachen:

• space after comma in the definition (pep8 conventions...)

• swap (default: False) and boolean in the docstring

• I would use the verb return instead of include

• is_central needs the appropriate doctests checking all possibilities.

• center needs an example when the hyperplane arrangement is not central.

I would suggest to use the Polyhedra parents and then return .universe() and .empty() which are less costly to execute in principle.

[git]

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

​57948da	cleaned up docstring of is_central
​5f16740	commented doctests for center method
​67a4ff7	use Polyhedra parents for universe and empty
​d72e65e	add doctest for is_central

[nailuj]

Replying to jipilab:

Here are few schönheitssachen:

• space after comma in the definition (pep8 conventions...)

Done.

• swap (default: False) and boolean in the docstring

Done.

• I would use the verb return instead of include

Done.

• is_central needs the appropriate doctests checking all possibilities.

Done.

• center needs an example when the hyperplane arrangement is not central.

Done.

I would suggest to use the Polyhedra parents and then return .universe() and .empty() which are less costly to execute in principle.

Done.

[jipilab]

No need of a bullet list for one element:

-        If ``certificate`` is False:
-
-        - A boolean
+        If ``certificate`` is ``False``, returns a boolean

• Missing empty lines before the doctests (in both functions).

• I would shorten the output of center to simply say that it is a polyhedron. If you want to be explicit, I would do it in the longer description before input/output.

[git]

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

​80b98c4

cosmetic changes

[nailuj]

Replying to jipilab:

No need of a bullet list for one element:

-        If ``certificate`` is False:
-
-        - A boolean
+        If ``certificate`` is ``False``, returns a boolean

Rephrased.

• Missing empty lines before the doctests (in both functions).

Inserted.

• I would shorten the output of center to simply say that it is a polyhedron. If you want to be explicit, I would do it in the longer description before input/output.

Moved.

[jipilab]

I noted another small thing:

+        If ``certificate`` is True, returns a tuple containing:
+
+        1. A boolean
+        2. The polyhedron defined to be the intersection of all the hyperplanes
+
+        If ``certificate`` is False, returns a boolean.

True and False should be between two back ticks... Otherwise, looks good. Just waiting that bots go over it again...

[git]

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

​c0b2f38

backticks for True and False

[nailuj]

Replying to jipilab:

I noted another small thing:

+        If ``certificate`` is True, returns a tuple containing:
+
+        1. A boolean
+        2. The polyhedron defined to be the intersection of all the hyperplanes
+
+        If ``certificate`` is False, returns a boolean.

True and False should be between two back ticks...

Done.

[tscrim]

A little doc change:

         - ``certificate`` -- boolean (default: ``False``); specifies whether
-        to return the center as a polyhedron (possibly empty) as part of the 
-        output.
           to return the center as a polyhedron (possibly empty) as part
           of the output

(Note also the lack of a period/full-stop and the removal of the trailing whitespace.)

Also, because of the @cached_method, there is no reason to do the computation twice. Constructing the certificate seems quite cheap to me, so I might instead put as the first lines:

if not certificate:
    return self.is_central(True)[0]

and then assume certificate=True for the rest. However, this really depends on how expensive that right_kernel is and how often you think the certificate will be requested. There is a balance to be had here and a judgement call to be made. I am just saying this is an alternative way to avoid doing the main computation twice should you ask for with a certificate and without.

[chapoton]

doc does not build

HyperplaneArrangementElement.is_central:9:
WARNING: Bullet list ends without a blank line; unexpected unindent.

[git]

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

​b74518a

corrected indentation in documentation

[nailuj]

Replying to tscrim:

A little doc change:

         - ``certificate`` -- boolean (default: ``False``); specifies whether
-        to return the center as a polyhedron (possibly empty) as part of the 
-        output.
           to return the center as a polyhedron (possibly empty) as part
           of the output

(Note also the lack of a period/full-stop and the removal of the trailing whitespace.)

Thanks, I was not careful enough with these conventions.

Also, because of the @cached_method, there is no reason to do the computation twice. Constructing the certificate seems quite cheap to me, so I might instead put as the first lines:

if not certificate:
    return self.is_central(True)[0]

and then assume certificate=True for the rest. However, this really depends on how expensive that right_kernel is and how often you think the certificate will be requested. There is a balance to be had here and a judgement call to be made. I am just saying this is an alternative way to avoid doing the main computation twice should you ask for with a certificate and without.

Thank you for sharing these thoughts. If both methods had been new, I probably would have done the right_kernel calculation and just decided to return it or not depending on the certificate switch. But as is_central was already implemented, I considered not slowing down the existing method more important than obtaining a faster computation of the new method center. Is this a good approach to deciding such issues?

[jipilab]

Replying to nailuj:

Replying to tscrim:

A little doc change:

         - ``certificate`` -- boolean (default: ``False``); specifies whether
-        to return the center as a polyhedron (possibly empty) as part of the 
-        output.
           to return the center as a polyhedron (possibly empty) as part
           of the output

(Note also the lack of a period/full-stop and the removal of the trailing whitespace.)

Thanks, I was not careful enough with these conventions.

Also, because of the @cached_method, there is no reason to do the computation twice. Constructing the certificate seems quite cheap to me, so I might instead put as the first lines:

if not certificate:
    return self.is_central(True)[0]

and then assume certificate=True for the rest. However, this really depends on how expensive that right_kernel is and how often you think the certificate will be requested. There is a balance to be had here and a judgement call to be made. I am just saying this is an alternative way to avoid doing the main computation twice should you ask for with a certificate and without.

Thank you for sharing these thoughts. If both methods had been new, I probably would have done the right_kernel calculation and just decided to return it or not depending on the certificate switch. But as is_central was already implemented, I considered not slowing down the existing method more important than obtaining a faster computation of the new method center. Is this a good approach to deciding such issues?

It sounds reasonable, yes. Though I actually like Travis's idea of not having to recompute the function, however it was called first. Is it hard to switch to this?

[nailuj]

Replying to jipilab:

Replying to nailuj:

Replying to tscrim:

Also, because of the @cached_method, there is no reason to do the computation twice. Constructing the certificate seems quite cheap to me, so I might instead put as the first lines:

if not certificate:
    return self.is_central(True)[0]

and then assume certificate=True for the rest. However, this really depends on how expensive that right_kernel is and how often you think the certificate will be requested. There is a balance to be had here and a judgement call to be made. I am just saying this is an alternative way to avoid doing the main computation twice should you ask for with a certificate and without.

Thank you for sharing these thoughts. If both methods had been new, I probably would have done the right_kernel calculation and just decided to return it or not depending on the certificate switch. But as is_central was already implemented, I considered not slowing down the existing method more important than obtaining a faster computation of the new method center. Is this a good approach to deciding such issues?

It sounds reasonable, yes. Though I actually like Travis's idea of not having to recompute the function, however it was called first. Is it hard to switch to this?

As Travis wrote, there are two lines to be inserted and some lines to be deleted, that would be all. If you think the additional computation time for the right_kernel and the construction of the Polyhedron aren't slowing down is_central too much, I could do it.

[jipilab]

Replying to nailuj:

Replying to jipilab:

Replying to nailuj:

Replying to tscrim:

Also, because of the @cached_method, there is no reason to do the computation twice. Constructing the certificate seems quite cheap to me, so I might instead put as the first lines:

if not certificate:
    return self.is_central(True)[0]

and then assume certificate=True for the rest. However, this really depends on how expensive that right_kernel is and how often you think the certificate will be requested. There is a balance to be had here and a judgement call to be made. I am just saying this is an alternative way to avoid doing the main computation twice should you ask for with a certificate and without.

Thank you for sharing these thoughts. If both methods had been new, I probably would have done the right_kernel calculation and just decided to return it or not depending on the certificate switch. But as is_central was already implemented, I considered not slowing down the existing method more important than obtaining a faster computation of the new method center. Is this a good approach to deciding such issues?

It sounds reasonable, yes. Though I actually like Travis's idea of not having to recompute the function, however it was called first. Is it hard to switch to this?

As Travis wrote, there are two lines to be inserted and some lines to be deleted, that would be all. If you think the additional computation time for the right_kernel and the construction of the Polyhedron aren't slowing down is_central too much, I could do it.

I would say it is fine yes.

Unless you find an example where you have tons of hyperplanes (10 000+) I don't think it will take too much time. Just for the record, if speed is really important, you could benchmark the before and after for some relatively big example just to have an idea...

[jipilab]

Hi Julian,

Are you going to do the suggested changes? It looks ok to me either way.

J-P

[nailuj]

Replying to jipilab:

Hi Julian,

Are you going to do the suggested changes? It looks ok to me either way.

J-P

Hi, I forgot about implementing the suggested changes. If you ask me like that, I would prefer the way it is written right now.

[jipilab]

Ok, sounds good.

I'm setting it to "needs review" to get another round of bots testing it, just for sanity reasons.

[tscrim]

(Morally) Green patchbots.

[jipilab]

Ok, may it be.