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

Replaced pyvista.wrap(alg.GetOutput()) with _get_output(alg). #2228

Merged
merged 4 commits into from
Mar 19, 2022

Conversation

harshi1122
Copy link
Contributor

Replaced pyvista.wrap(alg.GetOutput()) with _get_output(alg).
Can close #2226

@tkoyama010
Copy link
Member

Thank you for the contribution, it seems that Circular import is happening in CI. If you don't know how to solve it, please mention pyvista/developers.

@harshi1122 harshi1122 closed this Feb 17, 2022
@harshi1122 harshi1122 deleted the python-vista branch February 17, 2022 09:55
@harshi1122 harshi1122 restored the python-vista branch February 17, 2022 10:00
@harshi1122 harshi1122 reopened this Feb 17, 2022
@harshi1122
Copy link
Contributor Author

@tkoyama010
I don't know how to solve the CI issue
How do I mention the developers?

@adeak
Copy link
Member

adeak commented Feb 17, 2022

@harshi1122 thank you for the PR. The circular import was already addressed by akaszynski in the first version of your PR: #2227. I'm not sure why you closed that and opened a new one here :)

In the other PR you also asked about other uses of GetOutput() where a port is passed. If you look at the definition of _get_output() you can see that it supports additional parameters:

def _get_output(
algorithm, iport=0, iconnection=0, oport=0, active_scalars=None, active_scalars_field='point'
):
"""Get the algorithm's output and copy input's pyvista meta info."""
ido = algorithm.GetInputDataObject(iport, iconnection)
data = wrap(algorithm.GetOutputDataObject(oport))
if not isinstance(data, pyvista.MultiBlock):
data.copy_meta_from(ido)
if not data.field_data and ido.field_data:
data.field_data.update(ido.field_data)
if active_scalars is not None:
data.set_active_scalars(active_scalars, preference=active_scalars_field)
return data

But figuring out how those interact with non-trivial use cases is probably beyond the [good-first-issue] tag and it's alright if you don't touch those. I just wanted to address your question :)

@harshi1122
Copy link
Contributor Author

@adeak
Thanks a lot for your guidance and for answering my question.
I was having trouble pushing some commits to the other PR, and I messed up, so I decided to open a new one.
I have just recently started exploring Open Source so learning the ropes.
Thanks once again :)

@adeak
Copy link
Member

adeak commented Feb 17, 2022

What probably happened is that akaszynski pushed some commits to your branch, in which case you first have to pull the changes from your remote github repo and continue working from there.

If you need technical help with git (or anything related to contributing to PyVista) you can also find us in slack, you're welcome to join :)

@tkoyama010 tkoyama010 added the maintenance Low-impact maintenance activity label Feb 17, 2022
@banesullivan banesullivan marked this pull request as draft February 20, 2022 19:25
@akaszynski akaszynski marked this pull request as ready for review March 19, 2022 11:45
@codecov
Copy link

codecov bot commented Mar 19, 2022

Codecov Report

Merging #2228 (3770cb7) into main (eddd91a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2228   +/-   ##
=======================================
  Coverage   93.51%   93.51%           
=======================================
  Files          74       74           
  Lines       15737    15738    +1     
=======================================
+ Hits        14716    14717    +1     
  Misses       1021     1021           

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM

@akaszynski akaszynski merged commit 57db958 into pyvista:main Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using _get_output in wrapping alg output
5 participants