Cleanup of matrix_gfpn_dense

[simon-king-jena]

Ticket #21437 is supposed to be split into smaller chunks, and this is one of them: Make the code Python 3 compliant, use sig_on/sig_check/sig_off with more care, use the for i in range(bla) instead of for i from 0<=i<bla.

Depends on #21437
Depends on #23411

Component: packages: optional

Author: Simon King

Branch/Commit: c0fd902

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/23400

[simon-king-jena]

Branch: u/SimonKing/cleanup_of_matrix_gfpn_dense

[simon-king-jena]

Author: Simon King

[simon-king-jena]

Commit: 4cf1d50

[simon-king-jena]

comment:2

I suppose it is not needed to add a merge with the one commit from #23352 that is not already included in the branch from here.

---

New commits:

b1aef56	Add MeatAxe patch that ensures correct value of FfCurrentRowSizeIo
a44a58a	Fix reading mtx-matrix from non-existent file
91ac93f	Make pickling machine independent
225afe1	Properly fill the last few columns of random meataxe matrices
f6722be	Add c(p)def functions to get/set horizontal matrix slices
6f7caf9	Document and test _rowlist_()
8ebc4a6	Replace 'for...from' by '...in range()'
30e8793	Change some str into bytes
4cf1d50	Improve use of sig_check/on/off

[simon-king-jena]

comment:3

I really really really hate git's merging incapabilities.

[simon-king-jena]

Work Issues: rebase on top of #23399

[sagetrac-git]

Changed commit from 4cf1d50 to 0fb429b

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bd1f3f1	Fix ticket number in deprecation warning
35b4ee7	Merge branch 't/23352/meataxe_fixes' into t/23399/some_additions_to_matrix_mod_gfpn_dense
dac9f37	Replace 'for...from' by '...in range()'
b97839c	Change some str into bytes
6b6642c	Improve use of sig_check/on/off
0fb429b	Make matrix_gfpn_dense Python 3 compliant

[simon-king-jena]

comment:5

Since git wasn't able to merge the commit "Fix ticket number in deprecation warning", that merely replaces one number by another number in a single line, without touching anything else, I had to manually merge and subsequently rebase the branch.

I don't know if it is just me. Is there really no way to avoid dull manual work when all what I want to achieve is having a simple change in a single line?

[simon-king-jena]

Changed work issues from rebase on top of #23399 to none

[tscrim]

comment:7

Replying to @simon-king-jena:

Since git wasn't able to merge the commit "Fix ticket number in deprecation warning", that merely replaces one number by another number in a single line, without touching anything else, I had to manually merge and subsequently rebase the branch.

I don't know if it is just me. Is there really no way to avoid dull manual work when all what I want to achieve is having a simple change in a single line?

It sounds like you have two branches that have the "equivalent" commits but are based off two different branches. So they are not the same commits and git then has to deal with these conflicts. If the one particular commit with the change is what you wanted and are having these troubles, then you might want to cherry-pick the commit in.

Don't forget to also do git merge --abort when you want to stop doing a merge, but I think git is usually smart enough not to let you do really anything else in that state (although do not rely on this). Feel free to e-mail me if you have any git questions or want me to do something git related if you're having trouble too.

[simon-king-jena]

comment:8

Replying to @tscrim:

It sounds like you have two branches that have the "equivalent" commits but are based off two different branches. So they are not the same commits and git then has to deal with these conflicts. If the one particular commit with the change is what you wanted and are having these troubles, then you might want to cherry-pick the commit in.

No, this is not what my problem came from. And I did cherry-pick. See example below.

Don't forget to also do git merge --abort when you want to stop doing a merge,

I did.

Just for testing git, I created the following toy example:

• git init in some folder.
• create my_file, that contains the alphabet, one lower-case letter in each line. Add that file and commit. That's the starting point.
• create two branches A and B.
• In branch A, change the letter j in my_file to J (upper-case) and commit. Let's call this "commit xyz".
• In branch B, change all vowels to upper-case letters, and commit.

Now I am on branch B, and I want to apply the changeset from xyz. This could be tried by either "git merge A" or by "git cherry-pick xyz"; it turns out that the resulting problem is the same.

Of course the changeset from xyz cannot be applied, as the changeset's context contains one line with the lower-case letter i, but is being applied to a file where there is an upper-case letter I. So, of course, there is a conflict.

Therefore, after "git merge" resp. "git cherry-pick", we do "git mergetool". Result: meld opens with three versions of my_file. The left column is from branch B (vowels are upper-case, j is lower case). The middle column, which will eventually contain the result of the merge, is the common ancestor of A and B (all letters are lower-case). The right column is from branch A (only J is upper-case, the rest is lower-case).

Consequently, in order to merge, I need to manually change all vowels in the middle column to upper-case, and also change j to upper-case. In other words, I need to manually apply all changes.

And I really wonder why git makes me do 6 changes (5 vowels plus j), when 4 of these changes (namely the vowels a,e,o,u) are outside of the changeset.

To me, it seems obvious that in the middle column we should start with B (perhaps even after applying the hunks from the changeset that cleanly apply), because that's what we want to apply the changeset to. And then, we can solve the problem by focusing on the changeset: Change i into I and j into J. Done.

So, what is git's rationale for not doing the obvious? How to do better?

[tscrim]

comment:9

This is not git but the mergetool (which one are you using?). If you look at the actual file manually, you should see

<<<<<<< HEAD
I
j
=======
i
J
>>>>>>> A

(where A was the branch I was merging in) indicating that this was the only conflict git noticed. You can manually resolve the conflict, then run git add file and git commit to indicate you have resolved the merge.

Now my default mergetool meld does display the entire diff between the two files. However, it has an option to merge all non-confliciting changes, and I have to resolve manually the actual conflicts. It took me a little while to learn my mergetool, but now I am pretty good at it. However, git does do the conflict as you are expecting it to.

[simon-king-jena]

comment:10

Replying to @tscrim:

This is not git but the mergetool (which one are you using?). If you look at the actual file manually, you should see

<<<<<<< HEAD
I
j
=======
i
J
>>>>>>> A

I see. For me, it is

h
<<<<<<< HEAD
I
j
||||||| merged common ancestors
i
j
=======
i
J
>>>>>>> A
k

because I have this in my .gitconfig:

[merge]
        tool = meld
        log = true
        conflictstyle = diff3

When I remove the conflictstyle option, I get the same as you.

Now my default mergetool meld does display the entire diff between the two files. However, it has an option to merge all non-confliciting changes, and I have to resolve manually the actual conflicts. It took me a little while to learn my mergetool, but now I am pretty good at it. However, git does do the conflict as you are expecting it to.

Part of the problem may be this: I tried to replace meld with kdiff3. However, if I have this in .gitconfig

[merge]
        tool = kdiff3
        log = true

and do "git mergetool", then simply kdiff3 doesn't open. Do you know why?

[simon-king-jena]

comment:11

Aha! I found why kdiff3 didn't open: It silently did the job! See here

So, with the other configuration, "git mergetool" triggered kdiff3 to resolve the conflict automatically and save the result. So, I didn't need any manual intervention at all!

I will change to kdiff3, then...

[simon-king-jena]

comment:12

Argh.

After kdiff3's successful automatic merge, I did "git commit". To be on the safe side, I wanted to repeat the test. So, I did "git reset --hard HEAD~".

Then, the following happened when retrying:

$ git merge A
Auto-merging my_file
CONFLICT (content): Merge conflict in my_file
Resolved 'my_file' using previous resolution.
Automatic merge failed; fix conflicts and then commit the result.

These are conflicting messages: It says the it resolved the problem using previous resolution, but also stated that the automatic merge failed.

Therefore, I had a look at my_file, which looked fine: The conflict was correctly resolved. However, "git log" showed that the commit from A has in fact not been merged.

Therefore, I tried "git commit". Result:

$ git commit 
U	my_file
error: commit is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit.
fatal: Exiting because of an unresolved conflict.

Since there seem to be unresolved conflicts, I did

$ git mergetool 
No files need merging

WTF? There are unresolved conflicts, thus, I cannot commit, but no files need merging? Let's see the status:

On branch B
You have unmerged paths.
  (fix conflicts and run "git commit")

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   my_file

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	my_file.orig

Hm. Very very strange (to me, at least). Can you explain what is happening here?

[tscrim]

comment:13

I think what had happened is kdiff3 did not run git add file, which indicates to git that you have resolved the conflict, before you did your git commit. So I think that is how you ended up in this state. Try running

$ git add my_file
$ git commit

[simon-king-jena]

comment:14

Replying to @tscrim:

I think what had happened is kdiff3 did not run git add file, which indicates to git that you have resolved the conflict, before you did your git commit. So I think that is how you ended up in this state. Try running

$ git add my_file
$ git commit

Thank you! Yes, that did the trick.

OK. In future I will use kdiff3 instead of meld. Would you recommend to add "conflictstyle = diff3" to my .gitconfig, or better not use it?

[tscrim]

comment:15

Replying to @simon-king-jena:

Replying to @tscrim:

I think what had happened is kdiff3 did not run git add file, which indicates to git that you have resolved the conflict, before you did your git commit. So I think that is how you ended up in this state. Try running

$ git add my_file
$ git commit

Thank you! Yes, that did the trick.

No problem.

OK. In future I will use kdiff3 instead of meld. Would you recommend to add "conflictstyle = diff3" to my .gitconfig, or better not use it?

I don't use it for my workflow, but I would say it depends on what works best for you.

[simon-king-jena]

Work Issues: refactor a couple of tickets

[simon-king-jena]

Changed dependencies from #23399 to #

[simon-king-jena]

comment:17

I believe #23411, #23352 and #21437 should be stable now (although they still need a review), since the touched code should be clean. Thus, I am rebasing this ticket on top of the aforementioned (unless someone tells me to wait). Furthermore, I think #23399 (with further additions to meataxe) should be based on clean code, i.e., should be based on this ticket.

[simon-king-jena]

comment:18

Oops, #23411 needs work. Anyway, this ticket should be based on the others...

[simon-king-jena]

Changed dependencies from # to #23411 #23352 #21437

[simon-king-jena]

comment:20

Since #21437 depends on #23410, #23411 and #23352 anyway, I'm shortening the list of dependencies here. And then I'll resume work, as I believe that the dependencies should be more or less stable now.

[simon-king-jena]

Changed dependencies from #23411 #23352 #21437 to #21437 #23411

[simon-king-jena]

Changed dependencies from #21437 #23411 to #21437

[simon-king-jena]

comment:22

I don't know the policy about dependencies. #23411 is a dependency of #21437 and merges cleanly into #21437; however, the branch from #21437 doesn't contain all of #23411. Should it?

In any case, I believe the branch here should contain #23411. So, I'll start on top of #21437 with #23411 merged.

[simon-king-jena]

Changed dependencies from #21437 to #21437 #23411

[tscrim]

comment:23

IMO, it is easier for the reviewer to have all of the dependencies merged in. Although it does make it harder for looking at the diff. We do not have any concrete policy on the IIRC.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

e477948	Test meataxe unpickling being machine independent
01e2c0d	Mark new meataxe test 'optional'
d174835	Check bounds when unpickling meataxe matrices
8873330	Mark further meataxe tests optional; fix potential zero division
7a1ed5a	Meataxe unpickling: Further consistency checks
1c0dcd5	Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense
71abd44	Speedup meataxe matrix randomisation
e68140e	Replace sig_on/off by sig_check in meataxe matrix randomisation
82394bc	Merge branch 't/23352/fix_random_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense
ef9cfd3	matrix_gfpn_dense: Replace for...from by range(), cleanup use of sig_on/off/check, be Python3 compliant

[sagetrac-git]

Changed commit from 0fb429b to ef9cfd3

[simon-king-jena]

comment:25

The refactoring of meataxe tickets is now almost accomplished: Only #23399 (for the addition of new functionality) is left.

[simon-king-jena]

comment:26

Replying to @tscrim:

IMO, it is easier for the reviewer to have all of the dependencies merged in. Although it does make it harder for looking at the diff. We do not have any concrete policy on the IIRC.

Thank you. Here, I got a merge conflict (with #23352, not with #23411), and thus an explicit merge commit was needed anyway.

[tscrim]

Changed branch from u/SimonKing/cleanup_of_matrix_gfpn_dense to public/optional/cleanup_matrix_gfpn_dense-23400

[tscrim]

comment:27

LGTM. I did a trivial rebase, so I'm allowing myself to set a positive review.

---

New commits:

c0fd902

Merge branch 'u/SimonKing/cleanup_of_matrix_gfpn_dense' of git://trac.sagemath.org/sage into public/optional/cleanup_matrix_gfpn_dense-23400

[tscrim]

Changed commit from ef9cfd3 to c0fd902

[tscrim]

Changed work issues from refactor a couple of tickets to none

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

Changed branch from public/optional/cleanup_matrix_gfpn_dense-23400 to c0fd902