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

Hypercomplexes #2958

Merged
merged 9 commits into from
Nov 15, 2023
Merged

Hypercomplexes #2958

merged 9 commits into from
Nov 15, 2023

Conversation

HechtiDerLachs
Copy link
Collaborator

@jankoboehm : As discussed today. The relevant files are exclusively in experimental/HyperComplexes. The rest is in #2935 and can be ignored.

@HechtiDerLachs
Copy link
Collaborator Author

@jankoboehm : Did you have time to have a look at this? IMHO it would make sense to subsume double complexes under AbsHyperComplex and make them implement the hypercomplex interface. Then we could also introduce a concrete wrapper type for double complexes in parallel to the existing DoubleComplexOfMorphisms, which uses a hypercomplex in the background.

The same could then be done for ordinary ComplexOfMorphisms and even FreeResolutions. But being defined in Hecke, these can not yet be derived from AbsHyperComplex. So we probably do not touch upon this for the moment. What we could do nevertheless, is to rebase FreeResolutions on a concrete wrapper type for one-dimensional complexes which is based on the hypercomplexes here.

@HechtiDerLachs
Copy link
Collaborator Author

Freshly rebased for better readability.

@jankoboehm
Copy link
Contributor

I like it. It does what we discussed. Before interlinking it with double complexes, complexes or resolutions, I think we should get it it. I would then like to test it out in some use cases, also in the one-dimensional setting. We can then discuss carefully about interlinking it with what is there, ensuring compatibility. The goal is that the user does not have to struggle with different types of complexes not interoperating, or operating inconsistently.

@HechtiDerLachs HechtiDerLachs marked this pull request as ready for review November 1, 2023 15:26
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #2958 (16a261b) into master (a4e2994) will increase coverage by 0.00%.
The diff coverage is 73.39%.

@@           Coverage Diff            @@
##           master    #2958    +/-   ##
========================================
  Coverage   80.28%   80.28%            
========================================
  Files         483      484     +1     
  Lines       68262    68464   +202     
========================================
+ Hits        54803    54969   +166     
- Misses      13459    13495    +36     
Files Coverage Δ
...xperimental/DoubleAndHyperComplexes/src/Methods.jl 95.00% <ø> (ø)
...ental/DoubleAndHyperComplexes/src/vector_spaces.jl 90.24% <ø> (ø)
...ntal/DoubleAndHyperComplexes/test/DoubleComplex.jl 100.00% <ø> (ø)
...AndHyperComplexes/test/double_complex_interface.jl 100.00% <ø> (ø)
...ntal/DoubleAndHyperComplexes/test/vector_spaces.jl 100.00% <ø> (ø)
...mental/DoubleAndHyperComplexes/src/Constructors.jl 82.50% <82.50%> (ø)
...tal/DoubleAndHyperComplexes/src/tensor_products.jl 88.75% <81.57%> (ø)
...rimental/DoubleAndHyperComplexes/src/Attributes.jl 85.35% <77.92%> (ø)
experimental/DoubleAndHyperComplexes/src/Types.jl 50.42% <60.25%> (ø)

... and 2 files with indirect coverage changes

@HechtiDerLachs
Copy link
Collaborator Author

Rebased again after recent merge of the double complexes.

@HechtiDerLachs
Copy link
Collaborator Author

@jankoboehm : As discussed this morning. Hypercomplexes fused with the double complexes. For the fun of it, I also added a concrete type for double complexes which is wrapping a hyper complex. In the example in the test file it seems to perform slightly worse than the native double complex implementation. But since the latter is kept and used, I don't see a problem here.

@HechtiDerLachs
Copy link
Collaborator Author

Can this be merged then? Or should we talk about it again on Wednesday?

@fieker fieker merged commit c7f933b into oscar-system:master Nov 15, 2023
18 of 22 checks 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.

None yet

3 participants