Skip to content

Organizing the e directory#4347

Draft
MichaelABurr wants to merge 117 commits into
Macaulay2:developmentfrom
MichaelABurr:Subdirectories
Draft

Organizing the e directory#4347
MichaelABurr wants to merge 117 commits into
Macaulay2:developmentfrom
MichaelABurr:Subdirectories

Conversation

@MichaelABurr
Copy link
Copy Markdown
Member

These are the changes from the Macaulay2 workshop in Atlanta containing the changes to the locations of the files in the e-directory of Macaulay2.

mikestillman and others added 30 commits February 22, 2026 20:36
RingElem wraps (Ring*, ring_elem) with stack allocation and value-returning
operators, replacing verbose RingElement pointer code in tests. Includes
fromString via BasicPolyListParser. Also adds Monoid::variableNames().

I am using claude code opus 4.6 to help do this, in part to learn how to use claude effectively.
…g camel case instead of snake case for method names
First commit of a file that Mike gave us
Changed comments in NAG to use doxygen style
This reverts commit 766650f.
$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/Macaulay2/e/CoefficientRings>
$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/Macaulay2/e/matrices>
$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/Macaulay2/e/Rings>
$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/Macaulay2/c> # need scc-core.h
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this supposed to be rings, not Rings? Why are these here in the d directory, but only these?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that these should be removed (but we'll need to check after making this change). I believe that they were added as a work-around so that the full paths didn't need to be added, but we decided against that approach. It looks like we forgot to delete these after that decision.

@@ -8,13 +8,13 @@
#include <mpfi.h>
#include "interface/random.h"
#include "interface/gmp-util.h"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might need to move these 2 files out of interface/. But that should be done after this PR.

#include <vector>
// g++ franzi-brp.cpp franzi-brp-test.cpp -o franzi-brp-test
// g++ -I. computations/BRP.cpp computations/BRP-test.cpp -o BRP-test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did anyone test this line?

// Copyright 1998 Michael E. Stillman

#include "LLL.hpp"
#include "computations/LLL.hpp"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a comment, that might affect many files, including some (or many) before this PR. If a .hpp file is in the same directory as the .cpp file (as here), should it be, e.g., #include "LLL.hpp", or #include "computations/LLL.hpp". I'm not sure which is best. I have had trouble with CLion not being able to find header files, but that has largely been fixed by using compile_commands.json.

#include "MonomialTypes.hpp"
#include "PolynomialStream.hpp"
#include "../unused/PolynomialStream.hpp"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, if it is pointed to, is it unused? Potentially! This is fine though, just a comment.


#include "monideal-minprimes.hpp"
#include "text-io.hpp"
#include "../text-io.hpp"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be against the convention that files included are given paths off of e (if they are in this tree)?

#include "debug.hpp"
#include "monoid.hpp"
#include "text-io.hpp"
#include "../ExponentList.hpp"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment here. What do we want to do about .. in include files here?

@@ -453,7 +453,7 @@ const RingElement *towerExtendedGCD(const RingElement *F,
// top level translation to polynomials in other rings //
/////////////////////////////////////////////////////////

#include "polyring.hpp"
#include "rings/polyring.hpp"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a double include, ok I guess, and it predates this PR.

Comment thread CLAUDE.md
M2_arrayint arr = stdvector_to_M2_arrayint(std::vector<int>{2, 3, 5});
```

## Building and Running Tests
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This part is specific to me. We should fix this, but we can do so after this is merged, I believe.

BasicPoly parseBasicPoly(std::string poly, std::vector<std::string> varnames)
{
std::string_view str { poly};
std::string_view str {poly};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the (correctness of the )changes in this file tested?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Answering my own question!) Yes, I see that there is a new unit test file with a good set of tests.

@@ -0,0 +1,120 @@
// Copyright 2026, The Macaulay2 Authors.
//
// RingElem: A lightweight value-semantics wrapper around (const Ring*, ring_elem).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know I wrote this, but it was done with Claude's help. I can imagine changing this doc, but perhaps we do that separately to this PR?

@@ -0,0 +1,66 @@
// Copyright 2026, The Macaulay2 Authors.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This code is ok for now, but should be revisited when we are looking more specifically at the unit tests. (I know: I think I wrote it, or the first version of it).

Copy link
Copy Markdown
Member

@mikestillman mikestillman left a comment

Choose a reason for hiding this comment

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

Most of this PR involves name changes. Some involves doxygen doc, and some involve unit tests, and some involve removing complex.{c,h} files.

The refactoring group in Atlanta did a good job here, thanks!

Here are a few comments, to keep in mind going forward:

  • We want to be consistent with header file names: I've seen ../, but also just the name (where .. refers to e.)
  • there appears to be a file in unused which is begin used?
  • there are a few things flagged in comments. We should pay attention to these when we meet.
  • We should make a file in the issues directory (?) with a set of guidelines for naming of files here, how to do headers (as above), and maybe set up github to automatically format c++ code using .clang-tidy.

@mikestillman
Copy link
Copy Markdown
Member

I forgot that this is still a draft PR though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Engine Macaulay2/e

Projects

None yet

Development

Successfully merging this pull request may close these issues.