Refactoring from Atlanta conference#4346
Conversation
fix IM2 in matrix.h/.cpp
Turned IM2 names to raw
Group1: test refactoring!
…cing Matrix::promote
Push down 2/6 heavy-logic functions in interface/matrix.cpp to engine e/
Interface refactors
set JAVADOC_AUTBRIEF to YES
Clean up matrix.cpp in interface/
| F->append(deg); | ||
| } | ||
| return F; | ||
| return R->make_FreeModule(degs->len, degs->array); |
There was a problem hiding this comment.
I really like these simplifications in the interface directory, by the way!
| Build configurations are defined in `M2/BUILD/mike/Makefile`. Existing builds live | ||
| under `M2/BUILD/mike/builds.tmp/`. |
There was a problem hiding this comment.
I think either this CLAUDE.md file should be in M2/BUILD/mike or if it's in the repository root it should be usable to others.
There was a problem hiding this comment.
Looking at the blame, this seems to be from @mikestillman from 3 months ago. Mike, can you comment on these lines?
There was a problem hiding this comment.
Yes, I think this version of CLAUDE.md should not be here. This was grabbed from a branch I had for unit testing, that I had not intended to place here. I do think a good CLAUDE.md (or some other name that is less claude code specific) would be good here, but it shouldn't have my version of building M2 in BUILD/mike! in it. I can't recall why the library list is only some llibraries, I think it had to do something with what I was working on, but I forget now. even the top of the file is now out of date, after the "big sort"!
So let's delete it. AI USE disclaimer: On some of the unit tests code that the group took from me, I did use CLAUDE to help me write some of it. I probably want to modify it from the current state (it is slightly more verbose and complicated than needed, although to be fair, I did ask it to write some tests in a certain way (using fixtures), and it did that). I did modify some of the test code after claude code generated that, but some of it is un-edited, although the tests all pass.
There was a problem hiding this comment.
I agree with deleting CLAUDE.md at this point.
| - **FFLAS_FFPACK** (2.4.3+) - Finite field linear algebra routines (needs LAPACK, GIVARO) | ||
| - **GIVARO** (4.1.1+) - Prime field and algebraic computations | ||
|
|
||
| ### Libraries (found via find_package / FindXxx.cmake) |
There was a problem hiding this comment.
I'm not sure what's the source of this list, but it seems very incomplete (e.g. tbb and boost are missing). Perhaps we could just write that the list of dependencies should be read from cmake/check-libraries.cmake?
There was a problem hiding this comment.
I agree that it would be best for this to be a complete list or to not have the list at all. I believe that this also comes from @mikestillman .
There was a problem hiding this comment.
I think our group should write something like CLAUDE.md files in the e directory, and perhaps also in each of the directories in there, including the ones we have just created in the Subdirectories PR. But we need to curate those files, which we have not done yet. So they should not be in this PR.
mahrud
left a comment
There was a problem hiding this comment.
I did a brief review. Pretty much everything looks good.
- I tried rebasing to see if getting rid of the merge commits to linearize the history would be easy, sadly didn't seem so.
- I think CLAUDE.md should either be elsewhere or be applicable generally. Currently Mike's build process is hardcoded into it.
- I suspect the error failing indicates a behavior change or revealed bug, but I couldn't find it. Here's the message:
-- error log: /home/runner/work/M2/M2/M2/BUILD/build/usr-dist/common/share/doc/Macaulay2/ToricVectorBundles/example-output/_is__Vector__Bundle.errors:0:1:(3):[14]:
stdio:6:14:(3):[1]: error: first error occurred while lifting matrix entry at row 1, column 0|
Also, could you edit the top message to elaborate on AI use in this PR? |
|
@mahrud I agree that this ToricVectorBundles error may be new behavior. I'll start a bisect to try to find it. |
|
@mikestillman @andrew-tawfeek Could you add an AI statement to the pull request recording to what extent AI was used? |
|
Here's the example error we're getting: -- -*- M2-comint -*- hash: 8287022265155232415
i1 : E = toricVectorBundle(2,pp1ProductFan 2)
o1 = {dimension of the variety => 2 }
number of affine charts => 4
number of rays => 4
rank of the vector bundle => 2
o1 : ToricVectorBundleKlyachko
i2 : E = addBase(E,{matrix{{1,2},{3,1}},matrix{{-1,0},{3,1}},matrix{{1,2},{-3,-1}},matrix{{-1,0},{-3,-1}}})
o2 = {dimension of the variety => 2 }
number of affine charts => 4
number of rays => 4
rank of the vector bundle => 2
o2 : ToricVectorBundleKlyachko
i3 : isVectorBundle E
o3 = true
i4 : F = toricVectorBundle(1,normalFan crossPolytope 3)
o4 = {dimension of the variety => 3 }
number of affine charts => 6
number of rays => 8
rank of the vector bundle => 1
o4 : ToricVectorBundleKlyachko
i5 : F = addFiltration(F,apply({2,1,1,2,2,1,1,2}, i -> matrix {{i}}))
o5 = {dimension of the variety => 3 }
number of affine charts => 6
number of rays => 8
rank of the vector bundle => 1
o5 : ToricVectorBundleKlyachko
i6 : isVectorBundle F
stdio:6:14:(3):[1]: error: first error occurred while lifting matrix entry at row 1, column 0 |
|
I'd like to look further into the VectorBundle error. I think that we introduced this error. |
|
Just merged in |
These are the changes from the Macaulay2 conference C/C++ refactoring group. This focuses on documentation, unit tests, and changes to the interface directory.
(edit, by @mikestillman): AI disclaimer: Some of the unit test files were taken from a branch I was working on, and I used Claude code to help me write some of these tests. I will likely want to simplify some of the tests (I had asked it to use fixtures, which, although needed in one place, are generally not needed, and are somewhat more complicated that needed. All of the tests generated by AI (mostly in the Weyl algebras code, and util files being called there) were reviewed by me, and some were edited after code generation, but not all. The unit tests from this code all pass.