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

removed toolz, potential bugfixes, perfomance improvements, and more #43

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

jorenham
Copy link

I noticed that this repo was being used by some big performance-focused projects (e.g. pymc), so I figured I'd take a look at the code. So, apparantly, this is the result of me "looking" at the code 🤷🏻‍♂️ :

  • removed the need for toolz, by rewriting match.ordering using a (faster) dict comprehension
  • alleged performance improvement in util._toposort
  • alleged performance improvement in the variable.Var instance check
  • potential bugfixes regarding a couple of == comparison of type's
  • potential bugfix in case an immutable mapping is passed to core.assoc
  • avoid sorting dict items on python>=3.7 in utils.freeze
  • infinite-loop detection in util.transitive_get
  • some pep8 fixes, related to dictionary/tuple constructors
  • removed some redundant code from within try: blocks
  • trimmed some if statements

... and I'm sure I'm forgetting some stuff 😅

@brandonwillard
Copy link
Member

brandonwillard commented Jan 15, 2024

Thanks for the contribution! I'm pretty busy right now, so I might not get a chance to review this in the next few days, but the changes sound great, so I definitely do want to review it sooner than later.
In general, breaking each distinct change/improvement down into its own commit (or even PR) will help speed up the review a lot. If that's possible to do in the meantime, feel free to rebase this as much as you want. (I don't know if you've already done that, but it's worth mentioning just in case.)

@jorenham
Copy link
Author

@brandonwillard Except for a few (reasonable) exceptions, each commit is limited to the changes of one function.

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.

2 participants