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

Implement call method for elements in CDGA's #36329

Merged
merged 12 commits into from
Mar 25, 2024

Conversation

miguelmarco
Copy link
Contributor

Fixes #36328 by implementing the method.

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

While I am not 100% sure we should force the user to pass in every value, it is consistent with the multivariate polynomial ring.

I would also avoid code duplication for dict input and keyword input and just combine the two and go from there.

Having things made explicit in the doc about mixing input types and the expected behavior could be useful later on.

src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
Comment on lines +1607 to +1608
elif values:
raise ValueError("number of arguments does not match number of variables in parent")
Copy link
Collaborator

Choose a reason for hiding this comment

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

By this point, you have not used/parsed the keywords. So if someone passes in a mixture, then this might give a false result. In general, you probably should at least document the behavior and what you expect to have happen.

src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
@miguelmarco
Copy link
Contributor Author

I would also avoid code duplication for dict input and keyword input and just combine the two and go from there.

This is in some sense unavoidable, since in the case of dict, the keys are elements of the algebra, whereas in the case of keywords, the keys are strings. We could try to unify them, but then we would need code for that. It won't be really better than having two five-line loops that are close to duplicated.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

I probably would just make every dict comparison done by strings, but okay, let it be so once a few other very minor changes are done.

src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
@tscrim
Copy link
Collaborator

tscrim commented Feb 24, 2024

Thank you.

If you could just squash the (reviewer changes) commits, then it will be a positive review.

@miguelmarco
Copy link
Contributor Author

done!

Copy link

Documentation preview for this PR (built with commit 320f177; changes) is ready! 🎉

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you.

@vbraun vbraun merged commit b575734 into sagemath:develop Mar 25, 2024
17 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement __call__method for element of CDGA's
4 participants