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

add subroutine to delete arrays of tensors #155

Merged
merged 4 commits into from
Jul 10, 2024
Merged

Conversation

TomMelt
Copy link
Member

@TomMelt TomMelt commented Jul 5, 2024

This PR adds a subroutine (torch_tensor_array_delete) to delete arrays of tensors. This eliminate the need to manually loop over all elements of the array calling torch_tensor_delete, see e.g.,

call torch_tensor_delete(in_tensors(1))
call torch_tensor_delete(in_tensors(2))
call torch_tensor_delete(out_tensors(1))
call torch_tensor_delete(out_tensors(2))

Which now becomes,

call torch_tensor_array_delete(in_tensors)
call torch_tensor_array_delete(out_tensors)

  • add torch_tensor_array_delete to delete arrays of tensors
  • update tests
  • update docs

@TomMelt TomMelt added the enhancement New feature or request label Jul 5, 2024
@TomMelt TomMelt self-assigned this Jul 5, 2024
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

@TomMelt First glance looks good.

However, we'll need any examples in the documentation and readme also updating if you can go through these as well please?

I also wonder if your original suggestion of having an interface to delete both an array or a single tensor is a better to simplify the subroutine options. However, I also see that the explicitness of two different routines is more 'Fortranic'.

src/ftorch.f90 Show resolved Hide resolved
@TomMelt
Copy link
Member Author

TomMelt commented Jul 5, 2024

@TomMelt First glance looks good.

However, we'll need any examples in the documentation and readme also updating if you can go through these as well please?

done

@jatkinson1000
Copy link
Member

@TomMelt Thanks - I wonder if an addition to the API changes page to point out this new option might be useful, though not essential as the old function is preserved?

Also I added a comment above with an edit for your thoughts:

I also wonder if your original suggestion of having an interface to delete both an array or a single tensor is a better to simplify the subroutine options. However, I also see that the explicitness of two different routines is more 'Fortranic'.

Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Nice. Neatens things up a bit.

@jatkinson1000
Copy link
Member

A new thought on looking at some code today - perhaps we could have an interface torch_delete() that can take a model, tensor, or array of tensors and call the relevant deletion subroutine?

@TomMelt TomMelt merged commit d7c0f4d into main Jul 10, 2024
5 checks passed
@TomMelt TomMelt deleted the melt-delete-tensor-arrays branch July 10, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants