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

Fixed python depth limit; Introduce mixins. #60

Merged
merged 17 commits into from Feb 7, 2019

Conversation

krinart
Copy link
Member

@krinart krinart commented Feb 6, 2019

No description provided.

@coveralls
Copy link

coveralls commented Feb 6, 2019

Coverage Status

Coverage increased (+0.1%) to 95.373% when pulling 13b5562 on viktor/experiments_with_mixins into 5f5d99e on master.

@krinart krinart changed the base branch from viktor/expreminets_with_python_depth to master February 6, 2019 23:40
return re.sub("(?!^)([A-Z]+)", r"_\1", name).lower()


class BaseAstToCodeInterpreter(BaseAstInterpreter):
Copy link
Member

Choose a reason for hiding this comment

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

Why do we even need this intermediate entity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise mixins have attribute _cg which they are not aware of what it is.

from m2cgen.interpreters.interpreter import BaseAstToCodeInterpreter


class BinExpressionDepthTrackingMixin(BaseAstToCodeInterpreter):
Copy link
Member

Choose a reason for hiding this comment

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

For some reason I thought that mixins supposed to be outside of any particular class hierarchy. This is why they are "mixed in". In our case they are still inheriting the very base class. I'm not sure that it's that important, it just doesn't feel right :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise mixins have attribute _cg which they are not aware of what it is and IDE don't resolve methods of this attribute.

@krinart krinart force-pushed the viktor/experiments_with_mixins branch 2 times, most recently from 18bae56 to 2566351 Compare February 7, 2019 05:00
@krinart krinart force-pushed the viktor/experiments_with_mixins branch from 2566351 to 23c856b Compare February 7, 2019 05:05
@krinart krinart changed the title Experiments with mixins Fixed python depth limit; Introduce mixins. Feb 7, 2019
return re.sub("(?!^)([A-Z]+)", r"_\1", name).lower()


class BaseAstToCodeInterpreter(BaseInterpreter):
Copy link
Member

Choose a reason for hiding this comment

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

Was Ast remained here on purpose? Or did you just miss this one?

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


class BinExpressionDepthTrackingMixin(BaseAstToCodeInterpreter):
"""
This mixin provides an ability to call a custom cook when depth of the
Copy link
Member

Choose a reason for hiding this comment

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

cook -> hook? :)

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


It also provides flag `with_linear_algebra` which indicates whether
linear algebra was used during interpretation. It can be used to add
dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add corresponding third party dependencies that provide linear algebra operations and/or data structures?

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thanks!

@izeigerman izeigerman merged commit 30dc21c into master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants