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

Remove all occurences of VR.render() called right before VR.save() #2736

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

Xarthisius
Copy link
Member

This PR removes unnecessary and very costly .render() call, since Scene.save() explicitly calls Scene.render().

@Xarthisius Xarthisius added docs tests: running tests Issues with the test setup labels Jul 10, 2020
@Xarthisius
Copy link
Member Author

@yt-fido test this please

@chrishavlin
Copy link
Contributor

chrishavlin commented Jul 13, 2020

related: pull request #2743

the two PRs are compatible, but note that after 2743 is merged, doc/source/cookbook/sigma_clip.py could use render=False in the second and third calls to sc.save() since the sigma_clip is applied after rendering.

Copy link
Member

@munkm munkm left a comment

Choose a reason for hiding this comment

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

I totally agree with removing the .render() methods that are unnecessary in our test framework. I do think we don't have it clearly documented on the user side (unless I missed it) that the .save() method also calls render. Maybe adding a cell to our volume rendering tutorial would be a good place for to compare using .render + .show vs. .save? I think this would also help compliment the issue raised by #2733.

@Xarthisius
Copy link
Member Author

@yt-fido test this please

@matthewturk
Copy link
Member

@Xarthisius can you rebase or merge with master so we can run the tests?

@matthewturk matthewturk merged commit 46ab6c1 into yt-project:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs tests: running tests Issues with the test setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants