SpectralSequences / sseq

The root repository for the SpectralSequences project.
Apache License 2.0
25 stars 10 forks source link

Removing SteenrodAlgebraT traits #77

Open dalcde opened 2 years ago

dalcde commented 2 years ago

Are the SteenrodAlgebraT etc. traits still needed? They were introduced for the python bindings and I think they were added during a time where various things are hardcoded to accept SteenrodAlgebra only. Nowadays everything is properly polymorphic. Should we just remove these traits now?

@hoodmane

hoodmane commented 2 years ago

These are probably still needed for Python bindings to work. The Python bindings need all algebras to be concrete. But I'd have to think about it.

I have made some effort on a local branch to put Python bindings back in and I think I've gotten a good part of the way there but there's a lot of different things that are broken.

hoodmane commented 2 years ago

I guess it would be nice to get the bindings working before doing too much more cleanup so that no more stuff needs to change. On the other hand maybe it would be a simple change to adjust to using the replacement. It is also true from a trait logic perspective that SteenrodAlgebraT is pretty backwards.

dalcde commented 2 years ago

Can't we just make Box implement Algebra?

dalcde commented 2 years ago

Alternatively, expose SteenrodAlgebra and make an instance of PythonAlgera that just routes everything through SteenrodAlgebra. This should actually be more robust, and work for modules too

hoodmane commented 2 years ago

Well the point of SteenrodAlgebraT was that there are extra methods on the steenrod algebras which we also wanted to expose. But I guess you can dynamic_cast a dyn object? Maybe you try to dynamic_cast it to MilnorAlgebra and then try to dynamic_cast to AdemAlgebra then throw unsupported operation?

dalcde commented 2 years ago

Wouldn't that not be a problem if we use the route through PythonAlgebra strategy? I think this is also more semantically correct. The type system knows nothing about the algebra we are working with, but a specific instance of a PythonAlgebra contains implementatoin details of the algebra.

dalcde commented 2 years ago

We can also use the trait bound for<'a> &'a Self: Into<&'a SteenrodAlgebra> instead of inventing our own type.

hoodmane commented 2 years ago

Maybe. I think ideally we (I) should fix the Python bindings before making more changes, because they are already hard enough to fix. I suspect that you are correct, but it would be better to change the working bindings at the same time as making these other changes since the more the rest of the code changes away from the existing bindings code the harder it is to ever get them working again.

hoodmane commented 2 years ago

I guess I should make a PR with what I've done so far to fix the bindings so we can discuss.

hoodmane commented 2 years ago

Would be convenient if there were type coercions Matrix ==> MatrixSliceMut ==> MatrixSlice and similar for FpVector.

hoodmane commented 2 years ago

Probably using Box<dyn _> would work for everything better than the current approach and would reduce the amount of boilerplate by a lot.

dalcde commented 2 years ago

I'll look into replacing FiniteModule with Box<dyn Module>. This should have minimal performance impact since the bottlenecks do not involve the augmentation. It might be annoying to get the Yoneda stuff to work well though.