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

Allow more inputs for gens(::UniversalPolyRing, ...) #1809

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lgoettgens
Copy link
Collaborator

using @varnames_interface with a small workaround.

Due to weirdness with this macro between modules and inclusion order, I distributed the macro calls to respective files.

@lgoettgens lgoettgens force-pushed the lg/varnames-univ branch 2 times, most recently from cdba564 to 4d9f040 Compare September 26, 2024 13:12
@lgoettgens
Copy link
Collaborator Author

This seems to be breaking, as it changes the return value of gens(S, ["y", "z"]).
If nobody has an idea how to fix this, I'll be abandoning this in a few days

@fingolfin
Copy link
Member

What changes about the return value of gens(S, ["y", "z"]) ? Is it that it is now a tuple and would become a Vector ?
Ah I see, so code that does y,z = gens(S, ["y", "z"]) would then break?

In general it strikes me as being more consistent with what we do everywhere else: gens(R) always returns a Vector for our other rings. But yeah it'd be breaking

Perhaps a better API going forward would be this:

function (a::UniversalPolyRing)(b::VarName)
  return gen(a, b)
end

function (a::UniversalPolyRing)(b1::VarName, b2::VarName, bN::VarName...)
  return Tuple(gens(a, [b1, b2, bN...]))
end

Then one can write

julia> x = S(:x)
x

julia> y, z = S(:y, :z)
(y, z)

And here S(:y, :z) would indeed always return a tuple, even if gens might return a vector.

@fingolfin
Copy link
Member

However, we should also be a bit wary about breaking existing code. E.g. this would break the GenericCharacterTable package

@lgoettgens
Copy link
Collaborator Author

No, the problem is that gen(S, ["y", "z"]) used to return (y, z). Changing this to [y, z] would be mostly fine (haven't checked with GCT). But the way all of this varnames stuff works would change it to ([y,z],) (1-tuple of this vector), which is too weird to handle correctly

@fingolfin
Copy link
Member

This PR seems to do more than it says in the title: besides trying to tweak gens it also adds a bunch of @varnames_interface invocations in various places? Perhaps that could be separated out, as hopefully those are less problematic?

@fingolfin
Copy link
Member

I've asked @SoongNoonien to look into applying @varnames_interface to universal_polynomial_ring. We can also look into resurrecting this PR while doing that.

One idea might be to use a trick similar to what I did in Oscar for free_group, were I applied @varnames_interface to an internal helper function _free_group and then free_group mostly delegates to _free_group, but handles a few cases of arguments in a special way.

@lgoettgens
Copy link
Collaborator Author

This PR seems to do more than it says in the title: besides trying to tweak gens it also adds a bunch of @varnames_interface invocations in various places? Perhaps that could be separated out, as hopefully those are less problematic?

I've resurrected the first commit of this PR, resolved conflicts and put it into #1985.

I've asked @SoongNoonien to look into applying @varnames_interface to universal_polynomial_ring. We can also look into resurrecting this PR while doing that.

I don't think resurrecting this is worth the effort as one would first need to think how to make the approach here compatible with #1983. But feel free to take inspiration from this PR, if needed. And maybe you end up with something that covers this case here as well.

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.

2 participants