Skip to content

Add generic Ore polynomial module #2299

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rburing
Copy link
Contributor

@rburing rburing commented Apr 12, 2025

This contains only the basic structure so far, such as memory management, additive arithmetic, and multiplication from the left by an element of the base ring.

Feedback welcome!

We would like to use this data type as input to differential equation solvers, so we don't need the multiplicative structure initially, though it would be nice to have it in the future.

Multiplicative operations (to be defined later) depend on the Ore context object, so I figured that all operations should take the Ore context as input, to make the interface uniform. So now the interface looks like gr_mpoly, and the basic operations implemented so far call gr_poly methods.

Since the base ring will typically be polynomial, should gr_ore_poly_scalar_mul and GR_ORE_POLY_ELEM_CTX be named differently? Maybe using base / BASE?

I also tried to make the module "optional" in the sense that gr_mpoly is, let me know if I missed something in this regard.

cc @mezzarobba

@mezzarobba
Copy link
Contributor

mezzarobba commented Apr 12, 2025 via email

@rburing
Copy link
Contributor Author

rburing commented Apr 12, 2025

Some/most of the arithmetic/setter methods (gr_ore_poly_add for example) are not getting called in the ring tests.

I'm not sure what I did wrong since I did put them in the method table.

Edit: Never mind, I put some NULL pointers in there which is not allowed except to indicate the end of the table.

@rburing rburing force-pushed the gr_ore_poly branch 2 times, most recently from 4cfe741 to c5d790c Compare April 13, 2025 13:07
@fredrik-johansson
Copy link
Collaborator

So, working with Ore polynomials over say R[x], one might indeed want to do mixed operations with operands from either R or R[x]. The term "scalar" being ambiguous, instead of adding many extra methods, perhaps just define gr_ore_poly_add_other etc. that handles all cases?

@fredrik-johansson
Copy link
Collaborator

I don't see the need for ore_poly.h and ore_poly_types.h.

@fredrik-johansson
Copy link
Collaborator

Most of the one-line trivial methods could go in a single file.

@fredrik-johansson
Copy link
Collaborator

Multiplicative operations (to be defined later) depend on the Ore context object, so I figured that all operations should take the Ore context as input, to make the interface uniform.

Yes, this is the right way to do it. The way gr_poly and gr_mat work directly with the element context is an exception rather than the rule, and at some point we should add a public interface for those types using a poly/mat context as well.

@rburing
Copy link
Contributor Author

rburing commented Apr 18, 2025

I reorganized the code to collect the wrapper functions in one file, and applied all the other suggestions above. I removed all the methods with scalar in the name and added some other methods, indeed this looks much better.

Let me know if this needs any further changes.

@fredrik-johansson
Copy link
Collaborator

Looks good. The only thing missing is documentation.

@rburing rburing force-pushed the gr_ore_poly branch 2 times, most recently from e5ac464 to 6323582 Compare April 18, 2025 13:37
@fredrik-johansson
Copy link
Collaborator

Something that doesn't seem to be well specified in the documentation is the generator name (set_gen_name / string conversion methods). If I'm in R[x][D], say, shouldn't x already be named the base ring while the Ore ring generator is D?

@fredrik-johansson
Copy link
Collaborator

In fact, shouldn't the generator x (given as an element of the base ring) be a parameter to the Ore polynomial ring constructor? In general, the base ring could be something multivariate.

@rburing
Copy link
Contributor Author

rburing commented Apr 29, 2025

I updated the context and constructor to include an index of a generator of the base ring, and (hopefully) clarified the documentation. I think the restriction to generators of the base ring (rather than elements of the base ring) is reasonable, and e.g. in the univariate case it saves the user from having to define a variable that will probably never be used (e.g. multiplication by the variable will be replaced by a shift). Let me know if this is alright.

@rburing rburing requested a review from mezzarobba April 29, 2025 14:21
@rburing rburing force-pushed the gr_ore_poly branch 2 times, most recently from 5b16adc to ee4d57a Compare April 29, 2025 14:49
@mezzarobba
Copy link
Contributor

mezzarobba commented Apr 30, 2025 via email

@rburing
Copy link
Contributor Author

rburing commented May 5, 2025

Another question: How should the q parameter for q-shifts or q-derivations be handled? The current gr_ctx_init_gr_ore_poly and the struct _gr_ore_poly_ctx_struct do not refer to it. Can this be left for later? I guess the main question is: do we want to put an extra parameter (again an index into the list of generators or a gr_srcptr?) already in gr_ctx_init_gr_ore_poly, or add a separate second constructor later (or break source compatibility by adding an extra parameter to gr_ctx_init_gr_ore_poly later)?

@fredrik-johansson
Copy link
Collaborator

Either add a q-parameter which is allowed to be NULL, or add a separate constructor gr_ctx_init_gr_ore_poly_q_twist or whatever later for this case. I don't think it matters much which solution you choose.


/* This is mostly a copy of src/gr_poly/test/t-set_str.c */

TEST_FUNCTION_START(gr_ore_poly_set_str, state)
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be good to test that expressions like "Dx*x" either are correctly converted or correctly return GR_UNABLE.

@mezzarobba
Copy link
Contributor

I think the restriction to generators of the base ring (rather than elements of the base ring) is reasonable

There are definitely cases where one wants to work with derivations like x²·d/dx or operators relative to a generator of the base ring of the base ring, but they can probably be handled by a future “Ore twist as function pointer” mechanism.

@mezzarobba
Copy link
Contributor

Btw, what do you think about calling (σ,δ) the “(Ore) twist” instead of the “(Ore) operator”? I suppose “twist” would more properly refer to σ alone, but that would avoid some overloading of the word “operator”.

@mezzarobba
Copy link
Contributor

I'm also wondering if just gr_ore would be a better prefix than gr_ore_poly. OTOH gr_more for a potential future multivariate version may be confusing.

@rburing rburing force-pushed the gr_ore_poly branch 2 times, most recently from 47a2ff7 to e8fedc5 Compare May 15, 2025 16:18
@rburing
Copy link
Contributor Author

rburing commented May 15, 2025

Thanks for the extensive feedback @mezzarobba, I think it looks better now.

What do you think about using the name ore_algebra_t for the enum? With values ORE_ALGEBRA_STANDARD_DERIVATIVE and ORE_ALGEBRA_EULER_DERIVATIVE for now.

@fredrik-johansson Should I keep entry in the name of gr_ore_poly_entry_ptr for consistency with gr_poly, or use coeff instead?

@fredrik-johansson
Copy link
Collaborator

I think the gr_poly method is misnamed and should use coeff as well.

@mezzarobba
Copy link
Contributor

What do you think about using the name ore_algebra_t for the enum? With values ORE_ALGEBRA_STANDARD_DERIVATIVE and ORE_ALGEBRA_EULER_DERIVATIVE for now.

Sounds good too. I think I would call d/dx DERIVATIVE instead of STANDARD_DERIVATIVE, and you could add SHIFT (or maybe FORWARD_SHIFT) and FORWARD_DIFFERENCE right away.

This contains only the basic structure so far, such as memory
management, additive arithmetic, and multiplication from the
left by an element of the base ring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants