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

Resolve inconsistency with tensor strides and optimizer updates #71

Merged
merged 7 commits into from May 21, 2019

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented May 15, 2019

Adress #70

@NProkoptsev
Copy link

It seems copy_() takes more time than set_(), this will be more prominent if most of parameters will be without manifold constraints
You can check if tensors properties match each other

if point.stride() != new_point.stride():
    point.copy_(new_point)
else:
    point.set_(new_point)

@ferrine ferrine added priority: high Important PR bug-fix Bug fix related PR labels May 20, 2019
Copy link

@SomeoneSerge SomeoneSerge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just by the way... If I were doing it, I'd want to provide an explicit hint that one of the arguments is Tensor & and not const Tensor &, so perhaps a _ at the end: copy_or_set_().

is still awaiting for review

Well, really not the part that needed review

@ferrine
Copy link
Member Author

ferrine commented May 21, 2019

@newkozlukov, thanks, that is a better name for this. I skimmed over the codebase, looks like all critical places are covered with this function

@ferrine ferrine merged commit bd6c687 into master May 21, 2019
@ferrine ferrine deleted the contiguous-problem branch May 21, 2019 09:29
@ferrine
Copy link
Member Author

ferrine commented May 21, 2019

Thanks all

andbloch pushed a commit to andbloch/geoopt that referenced this pull request Dec 29, 2019
…pt#71)

* initial commit

* fix typo

* more efficient workaround

* fix some typos

* spacing in docs

* update changelog

* more intuitive name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Bug fix related PR priority: high Important PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants