Opened 4 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, GitHub, GitLab) | 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 4 years ago by
- Branch set to u/SimonKing/cleanup_of_matrix_gfpn_dense
comment:2 Changed 4 years ago by
- Commit set to 4cf1d50409eb1c8557eb6f77f58a95a6330904e4
- Status changed from new to needs_review
comment:3 Changed 4 years ago by
- 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 4 years ago by
- Commit changed from 4cf1d50409eb1c8557eb6f77f58a95a6330904e4 to 0fb429bfed28cb510aa51376f44fd634e52bbe33
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
|
comment:5 follow-up: ↓ 7 Changed 4 years ago by
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 4 years ago by
- Status changed from needs_work to needs_review
- Work issues rebase on top of #23399 deleted
comment:7 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 4 years ago by
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 4 years ago by
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: ↓ 10 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
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: ↓ 14 Changed 4 years ago by
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: ↓ 15 Changed 4 years ago by
Replying to tscrim:
I think what had happened is
kdiff3
did not rungit add file
, which indicates to git that you have resolved the conflict, before you did yourgit 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 4 years ago by
Replying to SimonKing:
Replying to tscrim:
I think what had happened is
kdiff3
did not rungit add file
, which indicates to git that you have resolved the conflict, before you did yourgit commit
. So I think that is how you ended up in this state. Try running$ git add my_file $ git commitThank 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 4 years ago by
- Status changed from needs_review to needs_work
- Work issues set to refactor a couple of tickets
comment:17 Changed 4 years ago by
- 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 4 years ago by
Oops, #23411 needs work. Anyway, this ticket should be based on the others...
comment:19 Changed 4 years ago by
- Dependencies changed from # to #23411 #23352 #21437
comment:20 Changed 4 years ago by
- Dependencies changed from #23411 #23352 #21437 to #21437 #23411
comment:21 Changed 4 years ago by
- Dependencies changed from #21437 #23411 to #21437
comment:22 Changed 4 years ago by
- Dependencies changed from #21437 to #21437 #23411
comment:23 follow-up: ↓ 26 Changed 4 years ago by
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 4 years ago by
- Commit changed from 0fb429bfed28cb510aa51376f44fd634e52bbe33 to ef9cfd315fac8e879163b58192c2bc8b6665c8b6
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
|
comment:25 Changed 4 years ago by
- 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 4 years ago by
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.
comment:27 Changed 3 years ago by
- 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:
c0fd902 | Merge 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
- Branch changed from public/optional/cleanup_matrix_gfpn_dense-23400 to c0fd902e57aa97eb1be16e03e786b7c0615ce09a
- Resolution set to fixed
- Status changed from positive_review to closed
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:
Add MeatAxe patch that ensures correct value of FfCurrentRowSizeIo
Fix reading mtx-matrix from non-existent file
Make pickling machine independent
Properly fill the last few columns of random meataxe matrices
Add c(p)def functions to get/set horizontal matrix slices
Document and test _rowlist_()
Replace 'for...from' by '...in range()'
Change some str into bytes
Improve use of sig_check/on/off