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

Modularization fixes for fast_callable interpreters #35774

Merged
merged 11 commits into from
Jul 1, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jun 15, 2023

πŸ“š Description

The code generator in sage_setup.autogen.interpreters can now build a subset of interpreters. This is used in #35095 to only build the interpreters for Element and object in the sagemath-categories distribution.

At runtime, sage.ext.fast_callable no longer fails when an interpreter cannot be imported but falls back to the Element interpreter if necessary.

πŸ“ Checklist

  • 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.

βŒ› Dependencies

@@ -174,7 +159,7 @@ def build_interp(interp_spec, dir):
write_if_changed(path, method())


def rebuild(dirname, force=False):
def rebuild(dirname, force=False, interpreters=None, distribution=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're adding arguments, can you add the missing INPUT block that explains what they do?

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 in 12c04b8

generate_code(et, str)
str.instr('return')
return builder(str.get_current())


def _builder_and_stream(vars, domain):
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see what's going on from the diff, but if someone comes along a year from now, this docstring isn't really going to explain what the function is doing and why. Can you expand it a little?

Also, if possible, can you test the import-failure code path?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just mechanical refactoring of code that was nested a bit too deep. Let me see if I remember what it is doing, after a mere 4 days, not a year...

Copy link
Member Author

Choose a reason for hiding this comment

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

can you test the import-failure code path?

done in afd3be6

@@ -60,14 +61,14 @@ def __init__(self):

Make sure that pow behaves reasonably::

sage: var('x,y')
sage: var('x,y') # optional - sage.symbolic
Copy link
Contributor

Choose a reason for hiding this comment

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

This should avoid the optional tag:

from sage.ext.fast_callable import ExpressionTreeBuilder
etb = ExpressionTreeBuilder(vars=('x','y'))
x = etb.var('x')
y = etb.var('y')
ff = fast_callable(x^y, domain=RDF)

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, done in 97135ad

@github-actions
Copy link

Documentation preview for this PR (built with commit 97135ad) is ready! πŸŽ‰

Copy link
Contributor

@orlitzky orlitzky left a comment

Choose a reason for hiding this comment

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

Ok, presumably you know what's going on with all_py, and the rest LGTM.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 19, 2023

Thank you! May I set to "positive review" then?

@orlitzky
Copy link
Contributor

Yes, it'll take me a few years to remember to do it.

@vbraun vbraun merged commit dc94b93 into sagemath:develop Jul 1, 2023
14 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 1, 2023
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.

None yet

3 participants