Opened 3 years ago

Closed 3 years ago

#23400 closed defect (fixed)

Cleanup of matrix_gfpn_dense

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-8.1
Component: packages: optional Keywords:
Cc: Merged in:
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: c0fd902 (Commits) Commit: c0fd902e57aa97eb1be16e03e786b7c0615ce09a
Dependencies: #21437 #23411 Stopgaps:

Description

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.

Change History (28)

comment:1 Changed 3 years ago by SimonKing

  • Branch set to u/SimonKing/cleanup_of_matrix_gfpn_dense

comment:2 Changed 3 years ago by SimonKing

  • Authors set to Simon King
  • Commit set to 4cf1d50409eb1c8557eb6f77f58a95a6330904e4
  • Status changed from new to needs_review

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:

b1aef56Add MeatAxe patch that ensures correct value of FfCurrentRowSizeIo
a44a58aFix reading mtx-matrix from non-existent file
91ac93fMake pickling machine independent
225afe1Properly fill the last few columns of random meataxe matrices
f6722beAdd c(p)def functions to get/set horizontal matrix slices
6f7caf9Document and test _rowlist_()
8ebc4a6Replace 'for...from' by '...in range()'
30e8793Change some str into bytes
4cf1d50Improve use of sig_check/on/off

comment:3 Changed 3 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to rebase on top of #23399

I really really really hate git's merging incapabilities.

comment:4 Changed 3 years ago by git

  • Commit changed from 4cf1d50409eb1c8557eb6f77f58a95a6330904e4 to 0fb429bfed28cb510aa51376f44fd634e52bbe33

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

bd1f3f1Fix ticket number in deprecation warning
35b4ee7Merge branch 't/23352/meataxe_fixes' into t/23399/some_additions_to_matrix_mod_gfpn_dense
dac9f37Replace 'for...from' by '...in range()'
b97839cChange some str into bytes
6b6642cImprove use of sig_check/on/off
0fb429bMake matrix_gfpn_dense Python 3 compliant

comment:5 follow-up: Changed 3 years ago by SimonKing

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?

comment:6 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues rebase on top of #23399 deleted

comment:7 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by tscrim

Replying to SimonKing:

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.

comment:8 in reply to: ↑ 7 Changed 3 years ago by SimonKing

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?

comment:9 follow-up: Changed 3 years ago by 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

(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.

comment:10 in reply to: ↑ 9 Changed 3 years ago by SimonKing

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?

comment:11 Changed 3 years ago by SimonKing

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...

comment:12 Changed 3 years ago by SimonKing

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?

comment:13 follow-up: Changed 3 years ago by 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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by SimonKing

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?

comment:15 in reply to: ↑ 14 Changed 3 years ago by tscrim

Replying to SimonKing:

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.

comment:16 Changed 3 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to refactor a couple of tickets

comment:17 Changed 3 years ago by SimonKing

  • Dependencies changed from #23399 to #

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.

comment:18 Changed 3 years ago by SimonKing

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

comment:19 Changed 3 years ago by SimonKing

  • Dependencies changed from # to #23411 #23352 #21437

comment:20 Changed 3 years ago by SimonKing

  • Dependencies changed from #23411 #23352 #21437 to #21437 #23411

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.

comment:21 Changed 3 years ago by SimonKing

  • Dependencies changed from #21437 #23411 to #21437

comment:22 Changed 3 years ago by SimonKing

  • Dependencies changed from #21437 to #21437 #23411

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.

comment:23 follow-up: Changed 3 years ago by 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.

comment:24 Changed 3 years ago by git

  • Commit changed from 0fb429bfed28cb510aa51376f44fd634e52bbe33 to ef9cfd315fac8e879163b58192c2bc8b6665c8b6

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

e477948Test meataxe unpickling being machine independent
01e2c0dMark new meataxe test 'optional'
d174835Check bounds when unpickling meataxe matrices
8873330Mark further meataxe tests optional; fix potential zero division
7a1ed5aMeataxe unpickling: Further consistency checks
1c0dcd5Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense
71abd44Speedup meataxe matrix randomisation
e68140eReplace sig_on/off by sig_check in meataxe matrix randomisation
82394bcMerge branch 't/23352/fix_random_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense
ef9cfd3matrix_gfpn_dense: Replace for...from by range(), cleanup use of sig_on/off/check, be Python3 compliant

comment:25 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

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

comment:26 in reply to: ↑ 23 Changed 3 years ago by SimonKing

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.

Last edited 3 years ago by SimonKing (previous) (diff)

comment:27 Changed 3 years ago by tscrim

  • Branch changed from u/SimonKing/cleanup_of_matrix_gfpn_dense to public/optional/cleanup_matrix_gfpn_dense-23400
  • Commit changed from ef9cfd315fac8e879163b58192c2bc8b6665c8b6 to c0fd902e57aa97eb1be16e03e786b7c0615ce09a
  • Milestone changed from sage-8.0 to sage-8.1
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review
  • Work issues refactor a couple of tickets deleted

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


New commits:

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

comment:28 Changed 3 years ago by vbraun

  • Branch changed from public/optional/cleanup_matrix_gfpn_dense-23400 to c0fd902e57aa97eb1be16e03e786b7c0615ce09a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.