sud0nick commented 5 years ago

See this thread for information regarding the changes: https://github.com/rlworkgroup/akro/issues/28 During your review please focus on test_dict.py and tuple.py.

The test case in the old tf and theano modules differs from the test case in the akro module. Please advise on which to keep.

Within the constructor for a Tuple it attempts to convert each component into a TF object and subsequently calls .dtypes on that object. For some reason, a TF object is never created (returns None) without any warnings or exceptions and I'm not sure why.

sud0nick commented 5 years ago

I just realized there are a lot of issues that need to be resolved. These weren't caught during the pre-commit and push checks on my system. I'll review them and resubmit later.

codecov[bot] commented 5 years ago

Codecov Report

Merging #33 into master will decrease coverage by 8.28%. The diff coverage is 69.56%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   67.24%   58.95%   -8.29%     
==========================================
  Files          15        7       -8     
  Lines         348      173     -175     
  Branches       61       19      -42     
==========================================
- Hits          234      102     -132     
+ Misses        105       66      -39     
+ Partials        9        5       -4

Impacted Files	Coverage Δ
src/akro/__version__.py	`0% <0%> (ø)`	:arrow_up:
src/akro/space.py	`57.14% <100%> (+4.2%)`	:arrow_up:
src/akro/discrete.py	`61.76% <100%> (-16.81%)`	:arrow_down:
src/akro/requires.py	`46.15% <46.15%> (ø)`
src/akro/box.py	`74.41% <66.66%> (-7.22%)`	:arrow_down:
src/akro/tuple.py	`46.15% <71.42%> (-40.21%)`	:arrow_down:
src/akro/dict.py	`62.96% <85.71%> (+22.48%)`	:arrow_up:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 24d9cf0...61b5527. Read the comment docs.

rlworkgroup / akro

Changes for issue #28 #33

Codecov Report