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

Copy matrep2 #141

Closed
Closed

Conversation

stevelinton
Copy link
Contributor

This code adds CopyToMatrixRep and friends and also forbids in-place conversion to matrix or vector reps of objects in the global region. This breaks quite a lot of code but needs to be done. I've fixed up the library I hope.

@stevelinton
Copy link
Contributor Author

The travis failures relate to a small change needed in autpgrp which is used in the morpheus tests. Not sure how to handle that.

@olexandr-konovalov
Copy link
Member

Does morpheus.tst work fine if autpgrp is not available?

@fingolfin
Copy link
Member

I can try to get Bettina to make an autpgrp releases with those fixes, if you either send me those fixes or commit them into the autpgrp repository yourself (I am pretty sure Bettina wouldn't mind).

if Characteristic(q) = 0 then
return fail;
fi;
q := Size(q);
Copy link
Member

Choose a reason for hiding this comment

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

What if q is a rational function field over a finite field? Then its characteristic is non-zero, yet its size is infinite. And we wouldn't know what to do with the matrix anyway. So perhaps use IsFFECollection(q) instead?

@olexandr-konovalov olexandr-konovalov added the topic: HPC-GAP Issues and PRs related to HPC-GAP label Sep 2, 2015
@markuspf
Copy link
Member

If we still want this I have a readily merged version on my machine now...

@stevelinton
Copy link
Contributor Author

I need to look at it and see if it still makes sense to me. I know my ideas moved on a little
part way through working on this, but I don’t recall the details.

Steve

On 12 Oct 2015, at 15:56, Markus Pfeiffer notifications@github.com wrote:

If we still want this I have a readily merged version on my machine now...


Reply to this email directly or view it on GitHub.

@fingolfin
Copy link
Member

So, what is the plan for this?

@fingolfin
Copy link
Member

@stevelinton What's the status of this? Do we still need it? My guess is yes... So, how do we proceed? What needs to be done? Who will do it? :-)

@fingolfin
Copy link
Member

Some thoughts:

  • we also need this in master, so that we can start converting the library uniformly to use this
  • we need to add tests
  • I have no experience using this, but for CopyToVector, I see a lot of code looking like this:
          if q <= 256 then
            w:=CopyToVectorRep( v, q );
            if w <> fail then
              v:=w;
            fi;
          fi;

I think it would be much better if one could just write this, unconditionally:

  v:=CopyToVectorRep( v, q );

We could retain a low-level variant with the current behavior, of course.

@stevelinton
Copy link
Contributor Author

This change is still needed, and I think it is the right choice to make this shift in master as well as hpc. Reformatting data in place will usually be slower than doing it as a copy and much more error-prone. HOWEVER, the old semantics let you sprinkle ConvertToVectorRep around like confetti, since it was idempotent and cheap once the vector was already converted. Some code may also depend on sharing semantics. So this needs to be done cautiously, with an eye for regressions.

I'm not promising to do anything at the moment -- still trying to catch up on what's happened while I've been out of the loop for six months or so, but I can answer questions about this if someone wants to try it.

@fingolfin
Copy link
Member

What do we do with this PR? I am tempted to just close it, as rebasing it likely is more effort than recreating it.

Also, I am not really happy about the current CopyToVectorRep -- the fact that the caller is responsible for checking beforehand whether it is valid to call it makes it very inconvenient to use. So before introducing that to plain GAP, perhaps we should come up with "better" semantics?

@stevelinton
Copy link
Contributor Author

Agreed to close this. On the semantics issue -- I agree subject to not costing too much in performance.

@fingolfin fingolfin closed this Sep 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: HPC-GAP Issues and PRs related to HPC-GAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants