Skip to content
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

Mayer-Vietoris #954

Merged
merged 5 commits into from
Sep 12, 2023
Merged

Mayer-Vietoris #954

merged 5 commits into from
Sep 12, 2023

Conversation

aljungstrom
Copy link
Contributor

This PR contains the Mayer-Vietoris sequence for cohomology with arbitrary coefficients. Basically directly copied from the ZCohomology version.

@aljungstrom aljungstrom mentioned this pull request Nov 28, 2022
@felixwellen
Copy link
Collaborator

Do you know that Cubical.Cohomology.EilenbergMacLane.MayerVietoris stops to type-check (with unsolved metas) when you remove --experimental-lossy-unification?

@aljungstrom
Copy link
Contributor Author

Ugh, yeah, I hadn't tried it for this version, but for the ZCohomology version I had the same problem. Didn't remember the unsolved metas, but I think type-checking is too slow even when you spell them out. My guess is that Agda is unfolding the definition of the cohomology group structure every time you try to define a homomorphism of cohomology groups.

@felixwellen
Copy link
Collaborator

I see. I don't know if anything bad can come from this.
Do you play around with "abstract" in situations like these?
For me, sometimes lossy-unification does it and sometimes abstract. In situations where both work, I prefer abstract, since that seems more controllable to me and it will have better support in the future (e.g. unfolding if you want to).
In case this is unclear: I'm not complaining and I have no idea if lossy-unifying things to something not really unique is bad.

@felixwellen
Copy link
Collaborator

We could just merge that, if you change "--experimental-lossy-unification" to "--lossy-unification" - it still checks on current master.

@felixwellen felixwellen self-assigned this Jul 10, 2023
@mortberg
Copy link
Collaborator

We could just merge that, if you change "--experimental-lossy-unification" to "--lossy-unification" - it still checks on current master.

Is this the only change needed to merge the PR?

@felixwellen
Copy link
Collaborator

Last time I checked it was

@mortberg
Copy link
Collaborator

mortberg commented Sep 12, 2023

Last time I checked it was

I fixed it. Looks like many files in the library still have experimental in them though...

@felixwellen
Copy link
Collaborator

Last time I checked it was

I fixed it. Looks like many files in the library still have experimental in them though...

No. Did you look at the version this PR is based on?

@mortberg
Copy link
Collaborator

Last time I checked it was

I fixed it. Looks like many files in the library still have experimental in them though...

No. Did you look at the version this PR is based on?

Oh yeah, so it will be fine once we merge?

@felixwellen
Copy link
Collaborator

Yes

@mortberg
Copy link
Collaborator

Yes

Great! I guess I've been too busy with other things for so long that I've forgotten how git works hehe

Copy link
Collaborator

@mortberg mortberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my fix let's merge this once the CI is happy

@felixwellen
Copy link
Collaborator

It shouldn't be, because the CI will use an old version of agda ;-)

@felixwellen
Copy link
Collaborator

If we want the CI to be happy, we will need to rebase onto master/merge master.

@felixwellen
Copy link
Collaborator

Ok, I guess that means I have a wrong idea about what the CI actually does.

@mortberg
Copy link
Collaborator

Ok, I guess that means I have a wrong idea about what the CI actually does.

Lol, you made me very confused just now...

@mortberg
Copy link
Collaborator

@felixwellen do you want to merge?

@felixwellen
Copy link
Collaborator

I don't get why it didn't fail, but let's just merge it - I have time to fix things, if it fails.

@mortberg
Copy link
Collaborator

I don't get why it didn't fail, but let's just merge it - I have time to fix things, if it fails.

I guess the changes are compatible with the state of master and the CI has confirmed that it doesn't fail after merging?

@felixwellen felixwellen merged commit e24f92c into agda:master Sep 12, 2023
1 check passed
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.

3 participants