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 two bugs: #1714

Merged
merged 1 commit into from
Feb 5, 2020
Merged

Fix two bugs: #1714

merged 1 commit into from
Feb 5, 2020

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Feb 4, 2020

  1. Buffer linkage - in debug builds, the static fields require external linkage (and explicit instantiation).
  2. Reshape - the tensors inside were reshaped, but the tensor list was not. Fixed now, using native TensorVector::Resize.

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Why we need this PR?

Pick one, remove the rest

  • It fixes two bugs:
  1. Buffer linkage - in debug builds, the static fields require external linkage (and explicit instantiation).
  2. Reshape - the tensors inside were reshaped, but the tensor list was not. Fixed now, using native TensorVector::Resize.

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    • Added explicit instantiation and out-of-class repetition of constexpr variable.
    • reshape.cc uses TensorVector::Reshap now
  • Affected modules and functionalities:
    • reshape.cc
    • buffer.h, added buffer.cc
  • Key points relevant for the review:
    ?
  • Validation and testing:
    • Old tests apply
  • Documentation (including examples):
    N/A

JIRA TASK: N/A

1. Buffer linkage - in debug builds, the static fields require external linkage (and explicit instantiation).
2. Reshape - the tensors inside were reshaped, but the tensor list was not. Fixed now, using native TensorVector::Resize.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient mzient requested a review from a team February 4, 2020 19:43
@mzient
Copy link
Contributor Author

mzient commented Feb 4, 2020

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1108498]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1108498]: BUILD PASSED

Copy link
Member

@szalpal szalpal left a comment

Choose a reason for hiding this comment

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

Looks OK, although I don't like the fact, that growth_factor and shrink_thresh are not private and have accessors (get and set). I'd make them private or remove the accessors. If you don't mind running CI again, you could change it

@JanuszL
Copy link
Contributor

JanuszL commented Feb 5, 2020

Looks OK, although I don't like the fact, that growth_factor and shrink_thresh are not private and have accessors (get and set). I'd make them private or remove the accessors. If you don't mind running CI again, you could change it

But they are protected as any other important member of Buffer class.

@mzient
Copy link
Contributor Author

mzient commented Feb 5, 2020

Looks OK, although I don't like the fact, that growth_factor and shrink_thresh are not private and have accessors (get and set). I'd make them private or remove the accessors. If you don't mind running CI again, you could change it

Accessors are indispensable - they are used in Python backend and in library initialization. As for making them private - that would be inconsistent with the rest of the Buffer class, and I don't think it's worth to move members between visibility levels when it doesn't really change anything. Maybe when/if we decide to revisit tensorlist/vector again (which we should), it will be time to think about refactoring buffer, too.

@mzient mzient merged commit 4101955 into NVIDIA:master Feb 5, 2020
@mzient mzient deleted the FixReshapeAndBufferLinkage branch March 27, 2020 13:40
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.

4 participants