Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ff: groebner bases for incremental ideals #9199

Merged
merged 5 commits into from Oct 10, 2022
Merged

Conversation

alex-ozdemir
Copy link
Member

No description provided.


std::unique_ptr<context::Context> d_context;

CoCoA::ring d_polyRing;
Copy link
Member

Choose a reason for hiding this comment

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

Can you give some quick doc on these members?

What context do they depend on? When are they computed? Etc.

void pop();

private:
void ensureSolution();
Copy link
Member

Choose a reason for hiding this comment

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

Doc.

private:
void ensureSolution();

std::unique_ptr<context::Context> d_context;
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. Documented.

EXPECT_EQ(ideal.hasSolution(), false);
}

#endif // CVC5_USE_COCOA
Copy link
Member

Choose a reason for hiding this comment

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

This has the same issue with the guard :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch 🤦

@alex-ozdemir
Copy link
Member Author

Thanks for the comment, Andy. Indeed the documentation was not up to scruff. Let me know what you think now, and whether you have more comments.

Copy link
Member

@ajreynol ajreynol left a comment

Choose a reason for hiding this comment

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

One minor comment, else LGTM.

#include <CoCoA/SparsePolyRing.H>
#include <CoCoA/ring.H>
#include <CoCoA/symbol.H>
#endif // CVC5_USE_COCOA
Copy link
Member

Choose a reason for hiding this comment

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

Like the other unit tests, let's just guard the whole file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. Good catch.

@ajreynol ajreynol self-requested a review October 7, 2022 21:47
@ajreynol ajreynol enabled auto-merge (squash) October 10, 2022 18:16
@ajreynol ajreynol merged commit 14a23d3 into cvc5:main Oct 10, 2022
@alex-ozdemir alex-ozdemir deleted the ff-groebner branch October 12, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
moderate Complexity normal Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants