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

Set pointer in example 6 to null #177

Closed
wants to merge 1 commit into from
Closed

Conversation

jatkinson1000
Copy link
Member

I believe this change resolves the issue originally seen in #166, again in #173, and investigated in #174, #175, #176.

After a pointer (pun intended) from @TomMelt towards initialising as null in #176 I conducted further investigations in #174
Checking the Fortran standards declaring a pointer has the default association state is undefined.
This is what we were doing in the autograd example with out_data.

When it is passed to torch_tensor_to array it could be unassociated, in which case the example runs as expected, registering this, and assigning/allocating the array.
However, it could also be associated, in which case this is picked up in torch_tensor_to_array, a mismatch in sizes registered, and an error raised.

The solution here is to ensure that we declare the pointer as null() at initialisation so that we avoid undefined behaviour.

This PR should fix the autograd example where it crashes in CI above, but future work by @jwallwork23 as he finalises this feature will need to make sure that there is good documentation to warn users that they need to be careful with pointers, and avoid undefined initialisation.

@jatkinson1000 jatkinson1000 requested review from jwallwork23, dorchard and TomMelt and removed request for dorchard, jwallwork23 and TomMelt October 8, 2024 17:02
@jatkinson1000 jatkinson1000 marked this pull request as draft October 8, 2024 17:05
@jatkinson1000 jatkinson1000 marked this pull request as ready for review October 8, 2024 17:18
@jatkinson1000
Copy link
Member Author

I believe the tests here are failing due to a regex mismatch in the result of the autograd example.
I have created a dummy PR which rebases @jwallwork23's work in #166 to fix this in on top of this.
This can be seen in #178 which passes the CI.

@jatkinson1000
Copy link
Member Author

I have just reviewed and seen that this is what @TomMelt did in #176 😅 but suggests not merging in favour of #175
I think it will be easiest to discuss this in person and reach a decision.

@jatkinson1000
Copy link
Member Author

See discussion in #175

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.

1 participant