Skip to content

145 add support for constructing from data without copying (ktensor) #182

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

Conversation

dmdunla
Copy link
Collaborator

@dmdunla dmdunla commented Jul 7, 2023

@dmdunla dmdunla self-assigned this Jul 7, 2023
@dmdunla dmdunla linked an issue Jul 7, 2023 that may be closed by this pull request
@dmdunla
Copy link
Collaborator Author

dmdunla commented Jul 7, 2023

The following methods currently return a copy make a copy of the ktensor:

  • permute
  • symmetrize

The following methods alter the underlying data members and do not make a copy:

  • arrange
  • fixsigns
  • normalize

Is this what we want? Which should be in-place (no copy) and which should make copies?

@dmdunla dmdunla requested a review from ntjohnson1 July 7, 2023 06:15
@dmdunla
Copy link
Collaborator Author

dmdunla commented Jul 7, 2023

@ntjohnson1 Please review changes to ktensor at this point. Once this has been reviewed, I'll move to tensor.

@ntjohnson1
Copy link
Collaborator

The following methods currently return a copy make a copy of the ktensor:

  • permute
  • symmetrize

The following methods alter the underlying data members and do not make a copy:

  • arrange
  • fixsigns
  • normalize

Is this what we want? Which should be in-place (no copy) and which should make copies?

We originally decided to match MATLAB's in-place vs copy choice. We are consistent with the methods you listed.

Returns copy

In-place

Copy link
Collaborator

@ntjohnson1 ntjohnson1 left a comment

Choose a reason for hiding this comment

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

Looks good to me. A few NITs you can address at your discretion. Always a fan of smaller PRs, my vote would be to merge this then open a second PR for tensor since the changes should be aligned in spirit but I doubt the implementation overlaps.

@dmdunla dmdunla changed the title 145 add support for constructing tensorktensor from data without copying 145 add support for constructing from data without copying (ktensor) Jul 7, 2023
@dmdunla
Copy link
Collaborator Author

dmdunla commented Jul 7, 2023

Will finalize this PR to address ktensor only in #145.

@ntjohnson1 Would you suggest breaking up #145 or just submitting several PRs for the individual classes?

@ntjohnson1
Copy link
Collaborator

Will finalize this PR to address ktensor only in #145.

@ntjohnson1 Would you suggest breaking up #145 or just submitting several PRs for the individual classes?

Whatever you think is most obvious. No strong preference. I mostly just wanted this to land today since I branched off it to start in on ktensor linting and typing here (finished linting and halfway through typing).

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.

Add support for constructing tensor/ktensor from data without copying
2 participants