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

Zero derivs for interactive params when needed #1700

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

aconty
Copy link
Contributor

@aconty aconty commented Jun 19, 2023

Sometimes an interactive param is used in a way that gets flagged as needing derivs. But we were neither allocating space for that nor zeroing them. This resulted on memory corruption.

Description

This fixes incorrect renders in a production context where params are often tagged as needing derivs.

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

Sometimes an interactive param is used in a way that gets
flagged as needing derivs. But we were neither allocating space
for that nor zeroing them. This resulted on memory corruption.

Signed-off-by: Alejandro Conty <aconty@imageworks.com>
@lgritz
Copy link
Collaborator

lgritz commented Jun 19, 2023

Hmmm, I think this will work, and I think it's fine to accept. But I wonder if a better fix long-term is that we should be turning derivatives off for a param that is marked as interactive. It won't have non-zero derivs, so any computations done using its derivative are pointless anyway.

@aconty
Copy link
Contributor Author

aconty commented Jun 19, 2023

I thought about that, but I wasn't familiar with all the moving parts and didn't try.

@lgritz
Copy link
Collaborator

lgritz commented Jun 19, 2023

I can take a look at that angle tomorrow when I'm back in the office.

If you need a workaround today for builds, it's fine to merge this, it doesn't hurt, I just think we can do better... if we can figure out the right spot to disable the derivs.

@aconty
Copy link
Contributor Author

aconty commented Jun 19, 2023 via email

@lgritz
Copy link
Collaborator

lgritz commented Jun 20, 2023

I'm going to merge this now so we can do an internal release, then I will come back to the problem afterwards and see if I can make it disable derivs entirely for these params that are marked interactive. (I don't want to hold up the fix waiting for me to get that right.)

@lgritz lgritz merged commit b4f0517 into AcademySoftwareFoundation:main Jun 20, 2023
20 of 23 checks passed
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.

None yet

2 participants