Implementation of Cohen-Macaulay test for simplicial complexes #11523

stumpc5 · 2011-06-20T14:56:12Z

Simplicial complexes were lacking a method to test Cohen-Macaulayness.

In order to implement this method, some other methods are improved, namely a hash function is implemented, and _enlarge_subcomplex has become faster.

For convinience, I also added a face_iterator.

Remark: the new line

int_facets = set( a.set().intersection(f_set) for a in new_facets )

in _enalarge_subcomplex improved speed for computing the homology by 65% (in the example I looked at -- needs to be verified). This method still has the potential to be speeded a lot, and it is responsible for a lot cpu time when computing the homology.

I also added a second version using parallel tests on multiple cpus.

Apply only: attachment: trac_11523-cohen_macaulay_complex-cs-jhp-review-ts.patch

Depends on #12587

CC: @jhpalmieri

Component: commutative algebra

Keywords: Cohen-Macaulay, homology, simplicial complexes

Author: Christian Stump

Reviewer: Travis Scrimshaw

Merged: sage-5.6.beta2

Issue created by migration from https://trac.sagemath.org/ticket/11523

The text was updated successfully, but these errors were encountered:

jhpalmieri · 2011-09-26T22:52:29Z

comment:3

I'm sorry, I forgot about this ticket.

Overall, I think this looks pretty good. Some comments and questions:

I had to rebase this against 4.7.2.alpha2.
why have both is_cohen_macaulay and is_cohen_macaulay_parallel? When would you ever want to use the first one? With ncpus=1, the parallel one is about as fast as the first, so should we get rid of the first one?
on the other hand, when I run the parallel version with ncpus=1, I sometimes see an error:

Exception OSError: (10, 'No child processes') in <generator object __call__ at 0x10d20ee10> ignored

and then the computation proceeds. Should we try to catch this and hide it? Can we do that if it says "ignored"?

in my testing (timeit('simplicial_complexes.NotIConnectedGraphs(5,2).homology()')), I saw no change in the time required to do homology computations from the old version to the new one, but if you have other ideas for speeding things up, please implement them.
the method face_iterator has no documentation.

I don't know if this was ready for review, but I'm attaching a new version of the patch which makes some of these changes: it renames is_cohen_macaulay to _is_cohen_macaulay_serial (so it's hidden), renames the parallel version to is_cohen_macaulay. I wonder if we should set ncpus to be 2 by default instead of 1? I'll try testing on a machine with just one cpu to see what effect that has.

stumpc5 · 2011-10-10T08:24:10Z

comment:4

Replying to @jhpalmieri:

Thanks for looking at it again, I hope we can get it into sage soon, as it was pretty much done. The only reason why I stopped working was that I thought I can still speed things up, but I never found the time to do so. I vote for leaving it as it is for now, and if someone (maybe me) needs a faster version, we get back to it.

Overall, I think this looks pretty good. Some comments and questions:

why have both is_cohen_macaulay and is_cohen_macaulay_parallel? When would you ever want to use the first one? With ncpus=1, the parallel one is about as fast as the first, so should we get rid of the first one?

that was just a left-over, as I first implemented is_cohen_macaulay and then started to play with the parallel version.

on the other hand, when I run the parallel version with ncpus=1, I sometimes see an error:
Exception OSError: (10, 'No child processes') in <generator object __call__ at 0x10d20ee10> ignored
and then the computation proceeds. Should we try to catch this and hide it? Can we do that if it says "ignored"?

If I remember right, this happens not only for 1 cpu, but if a negative answer is returned without waiting the other parallel sessions to be finished. I agree that we should catch that error!

in my testing (timeit('simplicial_complexes.NotIConnectedGraphs(5,2).homology()')), I saw no change in the time required to do homology computations from the old version to the new one, but if you have other ideas for speeding things up, please implement them.

See my comment at the beginning.

the method face_iterator has no documentation.

I will add it!

I don't know if this was ready for review, but I'm attaching a new version of the patch which makes some of these changes: it renames is_cohen_macaulay to _is_cohen_macaulay_serial (so it's hidden), renames the parallel version to is_cohen_macaulay. I wonder if we should set ncpus to be 2 by default instead of 1? I'll try testing on a machine with just one cpu to see what effect that has.

Thanks again, I will update your patch as soon as I have done the changes...

Best, Christian

stumpc5 · 2011-10-10T08:57:45Z

comment:5

I updated a new version. The only changes are that I import QQ as we need that, and I simplified the code for is_cohen_macaulay slightly. I saw that you added the documentation for face_iterator already, thanks!

stumpc5 · 2011-10-10T09:00:37Z

comment:6

Replying to @stumpc5:

I updated a new version. The only changes are that I import QQ as we need that, and I simplified the code for is_cohen_macaulay slightly. I saw that you added the documentation for face_iterator already, thanks!

It is rebased against 4.7.2.alpha3.

jhpalmieri · 2011-10-14T23:00:46Z

comment:7

This is mostly good. The only problems are with the parallel version: if I run with more than one process, I get a message like

sage: X.is_cohen_macaulay(2)
Killing any remaining workers...
False

If I run it with one process, I get

sage: X.is_cohen_macaulay(1)
Killing any remaining workers...
Exception OSError: (10, 'No child processes') in <generator object __call__ at 0x10ca39a00> ignored
False

We should be able to catch the error. Can we also suppress the message "Killing any remaining workers..."?

stumpc5 · 2011-10-15T05:10:17Z

comment:8

Replying to @jhpalmieri:

This is mostly good. The only problems are with the parallel version: if I run with more than one process, I get a message like
sage: X.is_cohen_macaulay(2)
Killing any remaining workers...
False
If I run it with one process, I get
sage: X.is_cohen_macaulay(1)
Killing any remaining workers...
Exception OSError: (10, 'No child processes') in <generator object __call__ at 0x10ca39a00> ignored
False
We should be able to catch the error. Can we also suppress the message "Killing any remaining workers..."?

I tried to catch it, but it it already caught somewhere inside the decorator, so I don't know how to suppress the output, do you?

Feel free to update the patch if you see a way to do it!

jhpalmieri · 2012-07-11T15:02:05Z

comment:9

I'm sorry this has languished for so long. Here is a new patch which turns off the message "Killing remaining workers...". I don't know how to get rid of the "No child processes warning", so I mentioned its possible presence in the docstring, so people won't be too concerned if they see it. I also removed the serial version altogether.

tscrim · 2012-10-27T19:11:44Z

comment:10

Because of the hashing of simplicial complexes, this is in conflict with #12587. I've started a topic on sage-devel on the mutability of SimplicialComplex.

stumpc5 · 2012-10-28T08:52:30Z

comment:11

Replying to @tscrim:

Because of the hashing of simplicial complexes, this is in conflict with #12587. I've started a topic on sage-devel on the mutability of SimplicialComplex.

Thanks for bringing this up. I will modify this ticket here as soon as the other ticket is ready.

tscrim · 2012-12-15T17:08:23Z

comment:12

Hey Christian,

Just wondering about the status of this patch now that #12587 is (code-wise) finalized.

Thanks,

Travis

stumpc5 · 2012-12-16T06:13:02Z

comment:13

Okay, this is now ready for its final review, I guess... Thanks Travis for bringing it up again!

tscrim · 2012-12-17T06:48:46Z

Reviewer: Travis Scrimshaw

tscrim · 2012-12-17T06:48:46Z

Dependencies: #12587

tscrim · 2012-12-17T06:48:46Z

comment:14

Attachment: trac_11523-cohen_macaulay_complex-cs-jhp-review-ts.patch.gz

Hey Christian and John,

This did not apply against #12587, so I did the very minor rebase in my review patch (it contains the entire patch; sorry if I overstepped here). I also made some minor docstring changes: some formatting and moved the note about the exception in a .. NOTE:: block. If you agree with the changes, feel free to set this to positive review.

Thanks for your work on this, I plan on putting this to good use,

Travis

For patchbot:

Apply only: trac_11523-cohen_macaulay_complex-cs-jhp-review-ts.patch

stumpc5 · 2012-12-17T10:04:27Z

comment:15

Replying to @tscrim:

Thanks for your work on this, I plan on putting this to good use,

great!

The patch looks good to me, so I am marking it positive...

Best, Christian

jdemeyer · 2012-12-23T21:17:11Z

comment:16

Is the work issue still relevant?

tscrim · 2012-12-23T21:25:07Z

Changed work issues from further improvement of _enlarge_subcomplex to none

tscrim · 2012-12-23T21:25:07Z

comment:17

Replying to @jhpalmieri:

Thanks for looking at it again, I hope we can get it into sage soon, as it was pretty much done. The only reason why I stopped working was that I thought I can still speed things up, but I never found the time to do so. I vote for leaving it as it is for now, and if someone (maybe me) needs a faster version, we get back to it.

It seems like it's not.

jdemeyer · 2012-12-27T10:24:46Z

Merged: sage-5.6.beta2

stumpc5 added this to the sage-5.6 milestone Jun 20, 2011

stumpc5 added c: commutative algebra labels Jun 20, 2011

stumpc5 assigned malb Jun 20, 2011

This comment has been minimized.

Sign in to view

stumpc5 added the s: needs review label Oct 10, 2011

This comment has been minimized.

Sign in to view

stumpc5 added s: positive review and removed s: needs review labels Dec 17, 2012

jdemeyer removed the s: positive review label Dec 27, 2012

jdemeyer closed this as completed Dec 27, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Cohen-Macaulay test for simplicial complexes #11523

Implementation of Cohen-Macaulay test for simplicial complexes #11523

stumpc5 commented Jun 20, 2011

This comment has been minimized.

This comment has been minimized.

jhpalmieri commented Sep 26, 2011

stumpc5 commented Oct 10, 2011

stumpc5 commented Oct 10, 2011

stumpc5 commented Oct 10, 2011

jhpalmieri commented Oct 14, 2011

stumpc5 commented Oct 15, 2011

jhpalmieri commented Jul 11, 2012

tscrim commented Oct 27, 2012

stumpc5 commented Oct 28, 2012

tscrim commented Dec 15, 2012

stumpc5 commented Dec 16, 2012

tscrim commented Dec 17, 2012

tscrim commented Dec 17, 2012

This comment has been minimized.

tscrim commented Dec 17, 2012

stumpc5 commented Dec 17, 2012

jdemeyer commented Dec 23, 2012

tscrim commented Dec 23, 2012

tscrim commented Dec 23, 2012

jdemeyer commented Dec 27, 2012

Implementation of Cohen-Macaulay test for simplicial complexes #11523

Implementation of Cohen-Macaulay test for simplicial complexes #11523

Comments

stumpc5 commented Jun 20, 2011

This comment has been minimized.

This comment has been minimized.

jhpalmieri commented Sep 26, 2011

stumpc5 commented Oct 10, 2011

stumpc5 commented Oct 10, 2011

stumpc5 commented Oct 10, 2011

jhpalmieri commented Oct 14, 2011

stumpc5 commented Oct 15, 2011

jhpalmieri commented Jul 11, 2012

tscrim commented Oct 27, 2012

stumpc5 commented Oct 28, 2012

tscrim commented Dec 15, 2012

stumpc5 commented Dec 16, 2012

tscrim commented Dec 17, 2012

tscrim commented Dec 17, 2012

This comment has been minimized.

tscrim commented Dec 17, 2012

stumpc5 commented Dec 17, 2012

jdemeyer commented Dec 23, 2012

tscrim commented Dec 23, 2012

tscrim commented Dec 23, 2012

jdemeyer commented Dec 27, 2012