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

first cut at showing multiple transfer functions on VR #2567

Merged
merged 30 commits into from Feb 14, 2022

Conversation

zingale
Copy link
Member

@zingale zingale commented Apr 27, 2020

PR Summary

Currently save_annotated() doesn't work correctly if we have multiple volume sources in the volume render. This is an attempt to fix that by looping over sources and showing all the transfer functions. With multiple transfer functions, we don't use the colorbar axis, but instead define a rectangle (tf_rect) in which to place the transfer functions. This is subdivided vertically to account for the number of render sources and the transfer functions are shown there. This gives some control over the location and sizing of the legends.

PR Checklist

  • Code passes flake8 checker
  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@zingale
Copy link
Member Author

zingale commented Apr 27, 2020

Here's an example output:

test_cb

@zingale zingale marked this pull request as ready for review April 28, 2020 14:54
@zingale
Copy link
Member Author

zingale commented Apr 28, 2020

updated output, with tf_rect manually changed to [0.8, 0.2, 0.08, 0.6]

test_cb

@munkm munkm added new feature Something fun and new! viz: volume rendering labels Apr 28, 2020
@zingale
Copy link
Member Author

zingale commented Apr 28, 2020

I've added a cookbook receipe demonstrating this, using the standard IsolatedGalaxy dataset. The render appears as:

test_cb

@zingale
Copy link
Member Author

zingale commented Apr 28, 2020

note also, in producing that example, I fixed a bug in the latex rendering -- the exponent was not places in {}

@zingale
Copy link
Member Author

zingale commented May 30, 2020

I just updated this with a fix for the case where not all the render sources have a transfer function (e.g., a volume source + a point source)

@zingale
Copy link
Member Author

zingale commented Jul 28, 2020

I've synced this up with master and verified that it still renders the two fields for the isolated galaxy correctly.

@zingale
Copy link
Member Author

zingale commented Oct 19, 2020

can I get some guidance on what people would like to see for this PR in order to get it merged?

@munkm
Copy link
Member

munkm commented Dec 4, 2020

Hi @zingale, I'm so sorry I dropped the ball on reviewing this. Would you mind merging or rebasing your PR with the current master branch so we can see how the tests look (and the conflicts are resolved)? I'm super excited about this feature and I think it will be a great addition to the project!

@munkm munkm self-requested a review December 4, 2020 17:31
Base automatically changed from master to main January 20, 2021 15:27
@neutrinoceros
Copy link
Member

neutrinoceros commented Jan 19, 2022

Wow it's been a while.
That said, the only 2 conflicts here are caused by auto-formatting. It should be trivial to fix.
@zingale do you still wish to get this merged ? As Madicken suggested, you'd need to update this branch somehow (rebase or merge), and then hopefully tests will stay green.

@zingale
Copy link
Member Author

zingale commented Jan 19, 2022

I'll sync this up today. Thanks for the poke

@neutrinoceros
Copy link
Member

A couple more fixes are needed. Most of them should be automatable with pre-commit.
Typing issues (mypy), not so much, but I can help there if needed @zingale :)

@zingale
Copy link
Member Author

zingale commented Jan 19, 2022

okay, I merged and fixed some issues due to volume rendering API changes, and the example works again:

render_two_fields_tf

@zingale
Copy link
Member Author

zingale commented Jan 20, 2022

I don't know about the runtime question -- it takes a few minutes on my workstation. But this is not the only VR of IsolatedGalaxy, so it can't be that much more expensive than existing ones in the cookbook.

@neutrinoceros
Copy link
Member

I don't know about the runtime question -- it takes a few minutes on my workstation. But this is not the only VR of IsolatedGalaxy, so it can't be that much more expensive than existing ones in the cookbook.

Fair enough

Co-authored-by: Clément Robert <cr52@protonmail.com>
zingale and others added 4 commits January 20, 2022 11:40
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
@zingale
Copy link
Member Author

zingale commented Jan 20, 2022

I think I've addressed all of the comments now

neutrinoceros
neutrinoceros previously approved these changes Jan 20, 2022
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Indeed. So I think we'd need a second approval to merge this. I'll advertise it on slack. Thank you @zingale !

Co-authored-by: Clément Robert <cr52@protonmail.com>
neutrinoceros
neutrinoceros previously approved these changes Jan 20, 2022
Co-authored-by: Clément Robert <cr52@protonmail.com>
neutrinoceros
neutrinoceros previously approved these changes Jan 20, 2022
@neutrinoceros
Copy link
Member

Looks like the docs build timed out. Let's hope that's not reproducible
@yt-fido test this please

@zingale
Copy link
Member Author

zingale commented Jan 21, 2022

I just noticed that I made the render big, so I'll reduce it to speed it up for the cookbook. I'll update in a sec.

@zingale
Copy link
Member Author

zingale commented Jan 21, 2022

okay, I made it reuse the existing render and made it smaller. At some point in the future, it would be good to separate the size of the rendering from the save_annotated image size, but I think that is a separate PR. I also fixed a bug from my merging that lost the ability to set the size of the transfer function labels. Now I can make them more appropriately sized.

@matthewturk matthewturk merged commit 55c2814 into yt-project:main Feb 14, 2022
@zingale
Copy link
Member Author

zingale commented Feb 14, 2022

thanks @matthewturk and @neutrinoceros !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants