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

Lean on compiler #215

Merged
merged 4 commits into from
Jan 6, 2025
Merged

Lean on compiler #215

merged 4 commits into from
Jan 6, 2025

Conversation

jwallwork23
Copy link
Contributor

Building in debug mode with gfortran 11.4.0 on Ubuntu 22.04, I came across several warnings, most of which are fixed in this PR. They are mostly minor but the ones to do with overloaded operators are an oversight of mine from #139.

@jwallwork23 jwallwork23 added the bug Something isn't working label Jan 6, 2025
@jwallwork23 jwallwork23 self-assigned this Jan 6, 2025
@@ -23,7 +23,6 @@ program example
real(wp), dimension(:,:), pointer :: out_data
real(wp), dimension(n,m) :: expected
integer :: tensor_layout(2) = [1, 2]
integer :: i, j
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused variables.

Comment on lines +296 to +301
do i = 1, ndims
if (i == 1) then
strides(layout(i)) = 1
else
strides(layout(i)) = strides(layout(i - 1)) * tensor_shape(layout(i - 1))
end if
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original version is a zero loop if ndims = 1.

Copy link
Member

Choose a reason for hiding this comment

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

Good spot!
I suppose it did what was intended, but the rewrite is much more explicit and doesn't leave us open to unexpected intentions in future.

@@ -38,7 +38,7 @@ subroutine main()
integer, parameter :: out_layout(out_dims) = [1, 2]

! Path to input data
character(len=100) :: data_dir
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Number of characters was inconsistent with number of characters for command line input.

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.

Thanks @jwallwork23 all looks sensible to me and some good catches.

Slightly worrying that the C precision was working in the old version...?

Happy for you to merge.

Comment on lines +296 to +301
do i = 1, ndims
if (i == 1) then
strides(layout(i)) = 1
else
strides(layout(i)) = strides(layout(i - 1)) * tensor_shape(layout(i - 1))
end if
Copy link
Member

Choose a reason for hiding this comment

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

Good spot!
I suppose it did what was intended, but the rewrite is much more explicit and doesn't leave us open to unexpected intentions in future.

@jwallwork23
Copy link
Contributor Author

Slightly worrying that the C precision was working in the old version...?

Agreed.

@jwallwork23 jwallwork23 marked this pull request as ready for review January 6, 2025 12:57
@jwallwork23 jwallwork23 merged commit d2e48b7 into main Jan 6, 2025
5 checks passed
@jwallwork23 jwallwork23 deleted the lean-on-compiler branch January 6, 2025 13:10
@jwallwork23 jwallwork23 added the autograd Tasks towards the online training / automatic differentiation feature label Jan 6, 2025
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.

2 participants