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

feat: Groebner Walk #571

Draft
wants to merge 108 commits into
base: master
Choose a base branch
from
Draft

feat: Groebner Walk #571

wants to merge 108 commits into from

Conversation

welpj
Copy link

@welpj welpj commented Apr 6, 2022

Implementation of the Gröbner Walk.

This branch contributes the following algorithms:

  • Gröbner Walk
  • Pertubed Gröbner Walk
  • Fractal Walk
  • Generic Walk
  • Tran´s version of the Groebner Walk

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Hi there,

thanks for your contribution. (Out of curiosity: who are you? Are you a member of the SFB-TRR 195 or outsider?)

I am afraid this PR will need some major revisions. E.g. just on the surface level: test related files should be in test/GroebnerWalk; runtests is missing a file extension; there are typos in filenames ("Utilitys" -> "Utilities"). But then also inside there are a structural issues.

Perhaps it'd be best to discuss this via a video chat instead of lengthy comments here.

@tthsqe12
Copy link
Contributor

tthsqe12 commented Apr 6, 2022

I would also suggest that Singular.jl is not necessarily the place such a large feature as this. Singular.jl is mainly for an interface to singular, and features like this belong in other packages. But, I am open to having my mind changed on this point.

@fingolfin
Copy link
Member

I just talked to @ederc on the phone who explained the background of this PR: @welpj is a master student of him and Anne. This PR will be revised. And he also said that perhaps it should rather go into Oscar.jl.

So, we can just leave this PR aside for the moment, until Anne and Christian tell us it is ready (resp. move it to Oscar.jl)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants