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

Fix copy/deepcopy for generic matrices #465

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Fix copy/deepcopy for generic matrices #465

merged 1 commit into from
Oct 8, 2019

Conversation

thofma
Copy link
Member

@thofma thofma commented Oct 7, 2019

@rfourquet
Copy link
Contributor

This works for generic matrices, but it removes copy on Nemo matrices... Sure this could be fixed by re-implementing copy on Nemo matrices, but this is unsatisfying to have to re-implement the same algorithm all over. I suggest we define a function which, given a matrix, returns an unitialized matrix of the same type over the same ring. I'm not sure how it should be called.

One option being to switch back similar to do just that: this very example shows that having similar return an object of the same type as the input when possible can be the desired behavior.

@thofma
Copy link
Member Author

thofma commented Oct 7, 2019

There is nothing unsatisfying with it. It will not be the same algorithm "all over" anyway since it will have to call into the C libraries. I will just add copy to Nemo matrices. We want this anyway, since the "generic" version is too inefficient.

@rfourquet
Copy link
Contributor

There is nothing unsatisfying with it.

It's better to be generic when possible. In this very PR, I agree this is not so terrible as you repeat the algorithm only twice. But besides Nemo, it would be nice to automatically provide copy and deepcopy for custom matrices (I don't know whether many custom matrices are/will be implemented though).

But if copy has to be re-implemented for Nemo matrices anyway, I suggest that we do just that and not touch the AA code, which works well as it is. Then we can document that if similar is implemented in a way that doesn't preserves the input type, then copy/deepcopy has to be implemented.

@thofma
Copy link
Member Author

thofma commented Oct 7, 2019

The function deepcopy for AbstractAlgebra matrices is broken once one does using Nemo. This is a bug in AbstractAlgebra. This PR here fixes it.

How would you fix this in Nemo? Implement copy(::Generic.MatSpaceElem{fmpz}) in Nemo? This is not an option.

@rfourquet
Copy link
Contributor

Implement copy(::Generic.MatSpaceElem{fmpz}) in Nemo?

Sure not, sorry I overlooked this issue. In this case, to avoid duplication between MatSpaceElem and MatAlgElem, you could also call _similar, which is like similar but guarantees to return the same element type.

I won't ask again, but you don't consider changing back similar right? In Base, copy(::AbstractArray) is defined in terms of similar, and in the docs we find statements like "guaranteeing that input and output are of the same type"... This seems to suggest that there is some kind of assumption about what similar returns that we don't support, even though it's not crystal clear in the docstring for Base.similar.

@thofma
Copy link
Member Author

thofma commented Oct 7, 2019

I have updated the PR to use _similar.

No I don't consider changing back similar in this way and I don't want to rehash this discussion.

@thofma thofma merged commit 6160ce4 into master Oct 8, 2019
@rfourquet rfourquet deleted the th/copy branch November 18, 2019 09:41
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.

copy(::MatrixElem) can return a different type
2 participants