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

Better pbw ideals arith #1591

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

tthsqe12
Copy link
Contributor

@tthsqe12 tthsqe12 commented Oct 4, 2022

Comment on lines 298 to 300
@doc Markdown.doc"""
pbw_algebra(R::MPolyRing{T}, rel, ord::MonomialOrdering) where T
pbw_algebra(R::MPolyRing{T}, rel, ord::MonomialOrdering; check = true) where T

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this now checked by default? Isn't this horribly expensive?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should discuss this in general and set a standard: speed over correctness or correctness over speed? Both have clear pros and cons...

Copy link
Member

Choose a reason for hiding this comment

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

Personally I prefer correct, slow results over wrong, fast results... but of course we all prefer correct & fast results... The question is whether to expect people to explicitly say "trust me, I am an expert" (so it's their own fault for running against a wall; but it our fault if things are "slow" if one does not know about check=false). On the other hand, will people who are more cautious expect our functions to not validate inputs by default? Hmmm...

So yeah, my preference actually is to have check=true the default everywhere and then expect everyone to explicitly say check=false if they are sure about it. But I also realize I may be a minority here :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that if the default is check = false, it makes the check argument useless. Then the only reason one would want to call a function with check = true is if one expects that the input is not valid. But then one should call the function which validates the input and not wait for an error in the constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not suggesting to start with check = false as default. Here is another example from the section on modules:

Warning

The functions do not check whether the resulting homomorphism is well-defined, that is, whether it sends the relations of M into the relations of N.

If you are uncertain with regard to well-definedness, use the function below. Note, however, that the check performed by the function requires a Gröbner basis computation. This may take some time.

is_welldefined(a::ModuleFPHom)

Return true if a is well-defined, and false otherwise.

I agree that we should agree on discussing this in general.

In the current example, we could do some time checks first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this horribly expensive?

No, it is just O(nvars^3) multiplications and additions in the algebra.

Copy link
Collaborator

Choose a reason for hiding this comment

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

O.k., thanks!

@tthsqe12
Copy link
Contributor Author

tthsqe12 commented Oct 4, 2022

By the way, I have no idea how to multiply two left ideals.

@wdecker
Copy link
Collaborator

wdecker commented Oct 4, 2022

@tthsqe12 Please replace

error("non-degeneracy condition is non-zero")

by

error("PBW-basis condition not satisfied")

@wdecker wdecker merged commit 0c355e1 into oscar-system:master Oct 5, 2022
wdecker added a commit that referenced this pull request Oct 6, 2022
fingolfin pushed a commit that referenced this pull request Oct 7, 2022
* Examples in Docu following PR #1591

* typo
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.

4 participants