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

Unit tests for operator overloading; fix scalar bug #225

Merged
merged 19 commits into from
Jan 13, 2025

Conversation

jwallwork23
Copy link
Contributor

@jwallwork23 jwallwork23 commented Jan 7, 2025

Closes #221.
Closes #224.

This PR implements unit tests for the overloaded operators acting on tensors using funit.

While implementing these tests, I discovered that pre/post multiplication by scalar, post-division by scalar, and scalar exponent don't work if the scalar is non-integer. This was an oversight by me, which is fixed in this PR. It turns out you can multiply/divide any tensor by a [1] tensor and torch treats it as a scalar, so that's what I do here.

In the case of exponentiation, it doesn't seem to be valid to have a tensor exponent. I revised the implementation to use a torch_scalar_t type and restricted attention to the integer case for now.

@jwallwork23 jwallwork23 added bug Something isn't working autograd Tasks towards the online training / automatic differentiation feature labels Jan 7, 2025
@jwallwork23 jwallwork23 self-assigned this Jan 7, 2025
@jwallwork23 jwallwork23 marked this pull request as ready for review January 7, 2025 17:07
@jwallwork23
Copy link
Contributor Author

Following our discussion in the meeting, I've now extended the power functionality to account for both integers and floats, replacing torch_scalar_t with torch_int_t and torch_float_t. I covered both cases with the square unit test and also added a square root one.

@jwallwork23 jwallwork23 force-pushed the 221_unit-test-overloads branch from b6cd616 to e03f75c Compare January 13, 2025 12:00
@jwallwork23
Copy link
Contributor Author

[Rebased on top of main to pick up the restructuring changes]

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.

Hi @jwallwork23 This looks great.
Am I correct that you've restructured the C++ to match the re-organising you did in the Fortran?

One observation (that is not to be resolved here) is that our online API docs are no longer very useful for basic users and are instead geared at developers.
In reality, all basic users want is the Interface docs, and the list they get above is somewhat daunting. There is an option to lost interfaces here but I don't feel it helps. I'm not sure if there is an easy solution to this and should perhaps create a new issue.

@jwallwork23
Copy link
Contributor Author

Am I correct that you've restructured the C++ to match the re-organising you did in the Fortran?

Oh, I didn't do that actually. Can do in a follow-up PR, though.

@jwallwork23
Copy link
Contributor Author

One observation (that is not to be resolved here) is that our online API docs are no longer very useful for basic users and are instead geared at developers. In reality, all basic users want is the Interface docs, and the list they get above is somewhat daunting. There is an option to lost interfaces here but I don't feel it helps. I'm not sure if there is an easy solution to this and should perhaps create a new issue.

That's a good point. I guess this is a downside of the fypp approach?

@jwallwork23 jwallwork23 merged commit d5046a3 into main Jan 13, 2025
5 checks passed
@jwallwork23 jwallwork23 deleted the 221_unit-test-overloads branch January 13, 2025 12:47
@jwallwork23
Copy link
Contributor Author

One observation (that is not to be resolved here) is that our online API docs are no longer very useful for basic users and are instead geared at developers. In reality, all basic users want is the Interface docs, and the list they get above is somewhat daunting. There is an option to lost interfaces here but I don't feel it helps. I'm not sure if there is an easy solution to this and should perhaps create a new issue.

Opened #232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autograd Tasks towards the online training / automatic differentiation feature bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator overloads not working correctly for non-integer scalars Unit tests for operator overloads
2 participants