Code cleanups

[Armavica]

Description

• use Counter and defaultdict where appropriate
• simplify boolean operations
• use list comprehensions where appropriate
• remove custom OrderedDict because dict is ordered since Python 3.7
• rewrite OrderedSet in terms of a dict

Related Issue

• Closes #
• Related to #

Checklist

• Checked that the pre-commit linting/style checks pass
• Included tests that prove the fix is effective or that the new feature works
• Added necessary documentation (docstrings and/or example notebooks)
• If you are a pro: each commit corresponds to a relevant logical change

Type of change

• New feature / enhancement
• Bug fix
• Documentation
• Maintenance
• Other (please specify):

[codecov]

Codecov Report

Attention: Patch coverage is 75.27473% with 45 lines in your changes missing coverage. Please review.

Project coverage is 81.01%. Comparing base (5c8afae) to head (7a7daac).
Report is 144 commits behind head on main.

Files with missing lines	Patch %	Lines
pytensor/misc/check_duplicate_key.py	0.00%	21 Missing ⚠️
pytensor/link/c/op.py	27.27%	8 Missing ⚠️
pytensor/compile/compiledir.py	0.00%	5 Missing ⚠️
pytensor/compile/debugmode.py	0.00%	2 Missing ⚠️
pytensor/compile/profiling.py	92.30%	2 Missing ⚠️
pytensor/graph/rewriting/basic.py	88.88%	2 Missing ⚠️
pytensor/compile/function/types.py	75.00%	1 Missing ⚠️
pytensor/graph/destroyhandler.py	91.66%	1 Missing ⚠️
pytensor/graph/fg.py	83.33%	0 Missing and 1 partial ⚠️
pytensor/scan/utils.py	66.66%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
+ Coverage   80.97%   81.01%   +0.03%     
==========================================
  Files         169      169              
  Lines       47015    46809     -206     
  Branches    11497    11475      -22     
==========================================
- Hits        38072    37923     -149     
+ Misses       6728     6679      -49     
+ Partials     2215     2207       -8     

Files with missing lines	Coverage Δ
pytensor/gradient.py	77.37% <100.00%> (ø)
pytensor/graph/basic.py	88.60% <100.00%> (-0.02%)	⬇️
pytensor/graph/features.py	65.22% <100.00%> (-0.09%)	⬇️
pytensor/graph/rewriting/db.py	85.96% <100.00%> (ø)
pytensor/link/basic.py	87.12% <100.00%> (-0.38%)	⬇️
pytensor/link/c/basic.py	87.48% <100.00%> (-0.14%)	⬇️
pytensor/link/c/cmodule.py	56.88% <ø> (ø)
pytensor/link/numba/dispatch/basic.py	86.20% <100.00%> (-0.15%)	⬇️
pytensor/link/numba/dispatch/elemwise.py	91.81% <100.00%> (-0.05%)	⬇️
pytensor/link/numba/dispatch/scan.py	95.86% <100.00%> (-0.03%)	⬇️
... and 25 more

... and 1 file with indirect coverage changes

[ricardoV94]

I wouldn't remove kanren if we don't have to. While the library itself (nor PyMC) is using it at the moment, it was added as a way to implement more powerful rewrites than what is possible with PyTensor built-in machinery. Since that's one of the strong points of PyTensor vs other tensor libraries, I would like to keep that around unless there's a strong objection?

[jessegrabowski]

It's sort of in the same category as egg-log -- something we could use but don't currently.

Maybe we need to discuss how/where/if it can be used in a future design meeting? I've asked @ricardoV94 about it in private a couple times and the response was always "idk, lol"

We could remove it after that meeting if we decide it's not worth it, or keep it otherwise.

[Armavica]

Sure, no worries. I'll debug what kanren doesn't like with the frozendict commit then.

[ricardoV94]

Otherwise I checked the rest of the PR and changes look fine. Had some doubts about replacing frozendict in the elemwise pattern, but I see how the less readable tuple may still be a good trade-off vs needing a frozendict implementation (custom or via a dependency).

Still a bit sad that it's not a thing in the Python standard lib

[ricardoV94]

If you find a bug in kanren, open an issue over there?

[Armavica]

@ricardoV94 I ended up reverting the removal of the custom frozendict implementation that we have. My analysis could be at least partially wrong, but I think the problem was the following:

• With this change, Elemwise.inplace_pattern went from an empty frozendict to an empty tuple
• this was creating a problem with kanren which expects some expressions as ExpressionTuples (or sometimes simple tuples), was assuming that this tuple was one of them, and crashing because it was empty.

Presumably, a fix would be to have kanren do a more precise type check isinstance(..., ExpressionTuple) instead of isinstance(..., Sequence), but this looks like more work in kanren than I have time to do now.

[ricardoV94]

What test was failing specifically?

[Armavica]

What test was failing specifically?

test_KanrenRelationSub_filters

[ricardoV94]

The empty tuple should be something we or kanren can fix. But let's leave that for later. Can you open a draft PR and linked issue with the frozendict removed so it's easier to investigate when we have the time? We can get the rest merged in the meantime

[Armavica]

Sure, I will do that when this one is merged (it's easier on top of this branch).