Adding bigraded Betti numbers functionality

[OP5642]

📚 Description

This is a part of my Google Summer of Code proposal, but also something that I wish to continue working on, regardless of the outcome of the proposal. My idea is the expand the library with functions from toric topology, which can be viewed combinatorially, and that is why they are of great interest among researchers in this field. So far I have added functions for computing bigraded Betti numbers, my further ideas can be seen in the proposal I recently sent.

📝 Checklist

• The title is concise, informative, and self-explanatory.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

#35410: An issue written by me, asking for the same things.

[tscrim]

@jhpalmieri You might be interested in this.

@OP5642 I would create an issue to keep track of the overall project. Specifically for this PR, I know it is still a draft, but it would be good if the documentation had a few more details about the definition. It should also be EXAMPLES:: (only one colon when there is text (not code) that follows) with a blankline after it.

[tscrim]

@OP5642 One other thing is to split the different parts of the project into multiple smaller PRs. This makes it easier to review, include in Sage, and track progress.

[OP5642]

Thank you for replying, @tscrim. If I understand correctly, should I perhaps rename the PR to bigraded Betti numbers, or something among those lines?

Also, I already created an Issue #35410, do you think I should create another one for tracking the overall progress of this PR?

Thank you for correcting me on the EXAMPLES:: part, I didn't catch that quite right the first time round, but I think I fixed it.

[jhpalmieri]

This change doesn't look right to me: if there is an indented block after "EXAMPLES", it should have double colons.

[OP5642]

My apologies, that was an accident. It should be corrected now.

[jhpalmieri]

I like the idea of adding this. Some comments: first there is code duplication between the two methods. Maybe bigraded_betti_number should do the work and bigraded_betti_numbers should just consist of a loop that calls the other method in order to produce the dictionary? If there are speed concerns, the results of bigraded_betti_number should perhaps be cached. (I don't know if speed is an issue.)

Second, Sage is inconsistent about Betti numbers: should they allow for different coefficients? Mostly they don't, but there are a few exceptions in the Sage library. Should we establish a policy?

[tscrim]

Thank you for replying, @tscrim. If I understand correctly, should I perhaps rename the PR to bigraded Betti numbers, or something among those lines?

Yes, that's correct.

Also, I already created an Issue #35410, do you think I should create another one for tracking the overall progress of this PR?

Consequently, I would just recycle #35410 into the general track-the-overall issue. IMO PRs do not need to have any specific issue tied to them if they are providing a new standalone feature or fixing a previously unreported bug.

+1 to @jhpalmieri's comments. I would cache the results as well, although one needs to be slightly careful with the mutability of simplicial complexes. I think the Betti numbers should allow for arbitrary coefficients; the current implementation in CellComplex just reads it off from the homology computation.

[OP5642]

Thank you for the suggestion about caching, @jhpalmieri and @tscrim. Speed is a concern, so I'll definitely look into that.

[OP5642]

I'm having a little trouble with the idea of caching here, @tscrim @jhpalmieri. Even if I were to call bigraded_betti_number() from bigraded_betti_numbers(), there would be no recurring calls with the same arguments, at least not when calling for a single complex. Maybe caching could help in the case where this is called for a large number of complexes and some subcomplexes coincide. However, after testing with some relatively large databases I have, I concluded that it is much faster as is. Maybe I was missing something or not caching correctly. Could you give me some guidance as to where I can further read about this? I was mainly using your online documentation, specifically sage.misc.cachefunc.CachedMethod.

[tscrim]

I find that speed claim suspicious as caching should have at worst a negligible impact on speed (if done properly). Caching only becomes useful if called multiple times, but it isn't good to compute things twice. Without knowing what you did, we have no idea what could be happening with your timings.

The other thing is that to remove the code duplication, we might be computing everything, whereas the user might only want one bigraded Betti number. This might not be a worthwhile tradeoff, but it would be good to remove as much duplication of the technical parts (so if there's a bug, it is only 1 fix instead of 2).

[OP5642]

I just pushed the (draft) changes, but I don't think this is what you asked for. As @tscrim pointed out, immutability was a problem, but I did not manage to solve it.

I'm still having trouble grasping the concept of caching here, since there will be no redundant calls. Can you maybe further elaborate on that idea?

I do understand the downsides of code duplication, but from what I've seen, it's much more efficient to compute inline. Perhaps I should consider combining these 2 functions into one and then having an optional argument, which would by default return the entire dictionary, but can also return the result with passed indices?

[jhpalmieri]

Re caching: maybe someone will compute bigraded_betti_number for a few values and then decide, I should just compute all of them and then call bigraded_betti_numbers. Without caching, those values would be computed twice. Without caching, an accidental second call to bigraded_betti_numbers would recompute everything.

Re code duplication: why not do something like

def bigraded_betti_number(self, i, j):
    ...code...

def bigraded_betti_numbers(self):
    d = {}
    for i in range(...):
        for j in range(...):
            d[(i,j)] = bigraded_betti_number(i,j)
    return d

(The details are probably wrong, but I hope the idea is clear.)

[tscrim]

@jhpalmieri I think the idea is to limit the number of times generated_subcomplex() and its homology() is called.

@OP5642 Based on this, what I would do is have this:

def bigraded_betti_numbers(self):
     if self._bbn is not None:
        return self._bbn
    # The main computation for all
    self._bbn = result
    return self._bbn

def bigraded_betti_number(self, a, b):
    if self._bbn is not None:
        return self._bbn.gen((a,b), ZZ.zero())
    # Do the single computation

with the mutation methods setting self._bbn = None (which would also be set in the __init__()).

Because of the optimization that is there in computing all of them, I don't think we can get around the code duplication too much. We could make those 5 lines of code into a separate method, but I am not convinced it is worthwhile right now (now that I've had time to look more closely at the code).

I will post some other specific changes, but a general comment is follow PEP8 and use lower_case for variable names when appropriate (things like H0 and S are fine).

[tscrim]

Note that you cannot just use @cached_method because the simplicial complex is mutable. You have to follow my previous comment to do the caching.

[OP5642]

Understood, I will try get to work on that.

[tscrim]

Thank you. I am approving this PR.

[OP5642]

Thank you! Is there anything else I should do now?

[tscrim]

For this ticket, nothing. However, you can work on another issue/PR.

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: 157384a

[vbraun]

CI fails, same when I try it locally.

sage -t --long --warn-long 38.4 --random-seed=123 src/sage/topology/simplicial_complex.py  # 3 doctests failed

[OP5642]

I am looking into it now. Seems strange, all errors are of this form:

Failed example:
    X.bigraded_betti_numbers()
Expected:
    {(0, 0): 1, (-1, 4): 2, (-1, 6): 1, (-2, 6): 1, (-2, 8): 1}
Got:
    {(-2, 6): 1, (-2, 8): 1, (-1, 4): 2, (-1, 6): 1, (0, 0): 1}

Those are the same dictionaries, but just differently sorted. Also, when I run this locally, I do get the dictionary as listed in the docstring (that is the way it is intended to be sorted). Any suggestions?

Edit: Also, I removed the trailing whitespaces, will push that now.

[jhpalmieri]

We can't guarantee any particular sorting on dictionaries, so you could change tests to something along the lines of

sage: X.bigraded_betti_numbers()[2] == (-1, 6)
True
sage: sorted(X.bigraded_betti_numbers().keys())
... (whatever the answer is) ...

etc.

[tscrim]

I thought our doctest output sorted dictionaries by keys, but perhaps I am misremembering? Python3 should also be consistent with the output because it is based on the order of the input. So I am a bit confused why this is failing.

Anyways, @jhpalmieri's fix will definitely work. You could also manually create the expected output dict and test against that too. Or

sage: sorted(X.bigraded_betti_numbers().items())

[vbraun]

The dict key order is insertion order in Python 3.7+, I'm guessing the algorithm has some non-determinism that randomly leads to different insertion order when the dictionary is constructed.

[OP5642]

I just pushed the changes. Now,

sage -t --long --warn-long 38.4 --random-seed=123 src/sage/topology/simplicial_complex.py

does not produce any errors. I opted for

sage: sorted(X.bigraded_betti_numbers().items(), reverse = True)

which does not produce a dictionary, but works fine (applying dict() to this messes with the sorting). Even though it may appear more organized, I think I should not manually change the expected output dictionary, I find it readable enough as is.

[tscrim]

One minor PEP8 thing: It should be reverse=True with no spaces.

[OP5642]

Yes, of course. I apologize, it should be fixed now.

[tscrim]

Thanks.