Skip to content

Changed Complex strict equality internally #3874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

mahrud
Copy link
Member

@mahrud mahrud commented Jun 5, 2025

This changes how two complexes are compared to be identical internally, without incurring any computational cost. The point is that these basic things didn't work before: (I added these as a test)

R = QQ[x]
C = complex R
D = complex R
id_C |  id_D
id_C || id_D
id_C // id_D

But now complexes are compared like so:

isIdentical(Complex, Complex) := (C, D) -> C === D or C.module === D.module and C.dd.map === D.dd.map

isIdentical(ComplexMap, ComplexMap) := (f, g) -> f === g or (
    f.degree === g.degree and f.map === g.map
    and isIdentical(f.source, g.source)
    and isIdentical(f.target, g.target))

Fixes #3865 and #3868

@mahrud mahrud requested review from mikestillman and ggsmith June 5, 2025 22:50
@ggsmith
Copy link
Contributor

ggsmith commented Jun 6, 2025

This looks like a good idea to me. However, should the method name be areIdentical?

@mahrud
Copy link
Member Author

mahrud commented Jun 6, 2025

Since it's an internal method I don't mind either way, but for the record I was going with matching isIsomorphic. Would you like me to change it?

@ggsmith
Copy link
Contributor

ggsmith commented Jun 9, 2025

Having investigated a bit further, I am still concerned that isIdentical is still too slow. In particular, it appears to take essentially the same amount of time as ==.

@mahrud
Copy link
Member Author

mahrud commented Jun 9, 2025

What was the example you tried it on?

@ggsmith
Copy link
Contributor

ggsmith commented Jun 10, 2025

@mikestillman and I create some relatively large Koszul complexes to compare isIdentical and ==. We also looked at the source code for === focusing on its behaviour for matrices and modules.

@mahrud
Copy link
Member Author

mahrud commented Jun 10, 2025

Ah, I see. Let me explain.

What you've observed about the difference between isIdentical and === is simply the fact that === for a HashTable (which like isIdentical checks === on all content) is slower than === for a MutableHashTable (which checks two fixed hash numbers)! In other words, even if you changed Complex into a HashTable, the timing would exactly match isIdentical, no better and no worse!

The reason in your examples == and isIdentical took the same amount is that internally == for modules and matrices first checks === before doing anything more complicated, and if you have large identical koszul complexes that === check succeeds immediately, which is exactly what isIdentical does, so they take the same amount. It's just that the matrices are large so checking entry by entry is taking a while.

In fact, in all cases simply constructing the koszul complex took an order of magnitude longer than isIdentical, so I'm not sure if the cost of using isIdentical is relevant at all. In other words, isIdentical is not going to be the bottleneck in a computation if constructing the objects in the first place takes an order of magnitude longer.

Finally, to respond to your initial concern, == and isIdentical are absolutely not the same when the modules are equal but not identical, which is the case you presumably want to avoid in liftMapAlongQuasiIsomorphism and other places. Here is an example:

debug needsPackage "FGLM"
I = cyclic(5)
C = complex module I
D = complex module ideal random I_*
elapsedTime isIdentical(C, D) -- false, .00001999s  elapsed
elapsedTime C === D           -- false, .000003078s elapsed
elapsedTime C == D            -- true,  .478898s    elapsed

If you raise that 5 to a 6 or 7, == should take up to half an hour!

@mahrud
Copy link
Member Author

mahrud commented Jun 26, 2025

To recap the conversation during the meeting, the example above shows that this statement about isIdentical is incorrect:

In particular, it appears to take essentially the same amount of time as ==.

I spent quite a bit of time with liftMapAlongQuasiIsomorphism and I could not produce an example where switching from === to isIdentical causes a noticeable slowdown. I would like to see an example if you have one in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants