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

Fix grad assignment and grad property read #6

Closed
wants to merge 1 commit into from

Conversation

Hofer-Julian
Copy link

Double assignment seems to do something different than expected in Python
Changing it to two statements fixes it.
This also reveals that the grad property is accessed at the wrong value.

Double assignment seems to do something different than expected
in Python
Changing it to two statements fixes it.
This also reveals that the `grad` property is accessed
at the wrong value.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@PiotrCzapla
Copy link

PiotrCzapla commented Nov 24, 2022

The issue is with map being a generator/iterator that you can use only once

>>> m = map(lambda x:x*10, [1,2,3])
>>> a,b,c = m
>>> test = m
>>> a, b, c
(10, 20, 30)
>>> test
<map object at 0x1046689a0>
>>> list(test)
[]

you can solve this by adding tuple in front of map

>>> test = a,b,c = tuple(map(str, [1,2,3]))
>>> test
('1', '2', '3')
>>> a, b, c
('1', '2', '3') 

But it is good catch, the double assignment is indeed quite problematic, as it doesn't work for some getitem implementations.

you can simplify the code a bit by saving to grads first, like so

ptgrads = w12,w22,b12,b22,xt2 = map(mkgrad, chks)

becomes:

ptgrads = tuple(map(mkgrad, chks))
w12,w22,b12,b22,xt2 = ptgrads

@jph00
Copy link
Member

jph00 commented Dec 5, 2022

Many thanks! I'll update it to the tuple version.

@jph00 jph00 closed this Dec 5, 2022
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.

3 participants