lf1-io / padl

Functional deep learning
Apache License 2.0
106 stars 4 forks source link

fix by checking for increment #432

Closed jasonkhadka closed 2 years ago

jasonkhadka commented 2 years ago

Description

Fixes #427

Checks for increasing the scopedname.n is implemented.

jasonkhadka commented 2 years ago

Check the continued discussion on the issue: https://github.com/lf1-io/padl/issues/427

wuhu commented 2 years ago

Thanks for this!

Could you explain why your changes fix the problem?

Also I found that it doesn't seem to work in combination with recursive function transforms, e.g.:

from padl import *
@transform
def f(x):
    return f(x-1)

f = f.pd_to('cuda')
print(f._pd_dumps())

raises an exception.

jasonkhadka commented 2 years ago

very strange!! Cause this works:

from padl import *
@transform
def f(x):
    return f(x-1)

print(f._pd_dumps())

I will have a look tomorrow, and also write the explanation.

from padl import *
@transform
def f(x):
    return f(x-1)

f = f.pd_to('cuda')
print(f._pd_dumps())
jasonkhadka commented 2 years ago

I have removed doctest on increament_same_name_var. The example is still there.

sjrl commented 2 years ago

@jasonkhadka What was wrong with the doctest?

jasonkhadka commented 2 years ago

find_in_source did not work with this doctest code. It is about finding the right scope. No fundamental issue here, just think we do not need doctest here.

@jasonkhadka What was wrong with the doctest?

sjrl commented 2 years ago

@jasonkhadka Could you add the examples @wuhu showed here https://github.com/lf1-io/padl/pull/432#discussion_r832318191 as unit tests? I think it would be good to test the behavior of the if else statement in question.

jasonkhadka commented 2 years ago

@jasonkhadka Could you add the examples @wuhu showed here https://github.com/lf1-io/padl/pull/432#discussion_r832318191 as unit tests? I think it would be good to test the behavior of the if else statement in question.

yes!

codecov-commenter commented 2 years ago

Codecov Report

Merging #432 (c4492de) into main (f258e5d) will decrease coverage by 1.49%. The diff coverage is 77.96%.

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
- Coverage   89.08%   87.59%   -1.50%     
==========================================
  Files          15       15              
  Lines        3006     3193     +187     
==========================================
+ Hits         2678     2797     +119     
- Misses        328      396      +68     
Impacted Files Coverage Δ
padl/dumptools/serialize.py 91.20% <ø> (ø)
padl/dumptools/ast_utils.py 30.00% <10.52%> (-25.82%) :arrow_down:
padl/dumptools/sourceget.py 89.23% <63.63%> (-2.58%) :arrow_down:
padl/dumptools/inspector.py 92.73% <66.66%> (-0.67%) :arrow_down:
padl/dumptools/symfinder.py 90.82% <88.75%> (+3.09%) :arrow_up:
padl/dumptools/var2mod.py 91.68% <88.88%> (-3.15%) :arrow_down:
padl/transforms.py 91.37% <97.22%> (-0.01%) :arrow_down:
padl/dumptools/packagefinder.py 96.61% <100.00%> (+0.05%) :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 0ab2d87...c4492de. Read the comment docs.

wuhu commented 2 years ago

@jasonkhadka did you the "5 hidden conversations"?

jasonkhadka commented 2 years ago

@wuhu hadn't seen them, but the refactor takes care of all the points.

@jasonkhadka did you the "5 hidden conversations"?

jasonkhadka commented 2 years ago

This is still not fixing this issue https://github.com/lf1-io/padl/issues/427

DEVICE = 'cpu'

@transform
def times_two(x):
    return x * 2

times_two.pd_to(DEVICE)
print(times_two._pd_dumps())

from padl import transform

@transform
def times_two(x):
    return x * 2

_pd_main = times_two