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

rename orderings: revlex->invlex,rp->ip...introduce deginvlex #722

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

hannes14
Copy link
Member

No description provided.

@fingolfin
Copy link
Member

Should we really do this here? It's a breaking change, and it is not really necessary for Oscar as far as I can tell (we don't expose the names used by Singular.jl in Oscar, do we?).

And arguably it makes sense for Singular.jl to use the same names as Singular does...

Project.toml Outdated
Statistics = "1.6"
julia = "1.6"
lib4ti2_jll = "1.6.10"
libsingular_julia_jll = "~0.40.5"
libsingular_julia_jll = "~0.40.6"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should first apply this in a separate PR, merge that and make a release?

Copy link
Member

Choose a reason for hiding this comment

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

I would support this separation.

Copy link
Member Author

Choose a reason for hiding this comment

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

done with #723

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.

I think we need to discuss this some more. Hopefully @wdecker can also comment.

Let me emphasize that this is really independent of the discussion about how to name monomial ordering in Oscar: The Oscar names are completely independent from the names in Singular.jl. If we change names in Singular.jl, that just causes more work on the Oscar side. That means: if you really want it, we can do it; but otherwise I'd rather keep the names here and focus on improving Oscar.

@@ -81,13 +81,14 @@ JLCXX_MODULE define_julia_module(jlcxx::Module & Singular)
/* monomial orderings */
Singular.set_const("ringorder_no", ringorder_no);
Singular.set_const("ringorder_lp", ringorder_lp);
Singular.set_const("ringorder_rp", ringorder_rp);
Singular.set_const("ringorder_ip", ringorder_ip);
Copy link
Member

Choose a reason for hiding this comment

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

But the order is called "rp" in Singular, isn't it? If so, I don't support this part of the PR. Same for rs -> is.

That leaves the addition of Ip. Is that what it is called in Singular? Or is it perhaps Rp there? Whatever it is, we should use the name used in Singular here.

@wdecker please comment if you disagree

Copy link
Member

@ederc ederc Nov 3, 2023

Choose a reason for hiding this comment

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

I am confused: AFAIK the naming convention should also be changed inside Singular itself. So in order to have Singular.jl coincide with Singular these changes should be done, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The orderings in Singular are 1- or 2-letter-codes, names appear only in the manual.
In Singular.jl main orderings are denoted by names (like :degrevlex).
The orderings in question (in Singular: rp/rs) have a warning in the manual
(only introduced to make "opposite ring" possible).
The ordering "deginvlex" (Ip in Singular) is new - so here we are completely free with the naming.

src/poly/orderings.jl Outdated Show resolved Hide resolved
ordering_Ip(nvars::Int = 1)

Represents a block of at least `nvars` variables with the
degree inverse lexicographical ordering (:deginvlex).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe adjust this docstring, too?

ordering_is(nvars::Int = 1)

Represents a block of at least `nvars` variables with the
negative inverse lexicographical ordering (:neginvlex).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe adjust this docstring, too?

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.

@wdecker explained that Singular will also rename rp to ip. Thus the changes here make sense in that context

@fingolfin
Copy link
Member

Rebased this, and changed the version to 0.20.0-DEV, as the change in here is breaking.

currently we have both: ordering_ip and ordering_rp (also is/rs).
The description refers to the upcoming naming (invlex/ip)
@hannes14 hannes14 merged commit 2b8a7d5 into oscar-system:master Nov 7, 2023
12 of 14 checks passed
@hannes14 hannes14 deleted the hs/orderings branch November 7, 2023 12:57
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