Skip to content

[fix] fix for output that are numpy.array (closes #270, #339) #340

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[fix] fix for output that are numpy.array (closes #270, #339) #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pydra/engine/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def copyfile_workflow(wf_path, result):
for field in attr_fields(result.output):
value = getattr(result.output, field.name)
new_value = _copyfile_single_value(wf_path=wf_path, value=value)
if new_value != value:
if not (new_value is value or new_value == value):
Copy link

Choose a reason for hiding this comment

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

I am afraid that this won't work for numpy arrays new_value and value that are different from each other.

It will result in a boolean array (same size results in elementwise comparison) or raises an exception in the future (elementwise comparison is deprecated for arrays of different sizes).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is why I added new_value is value part for numpy arrays.
Do you have an example that doesn't work with it? Could you please post it. I believe the one you send in #270 should work (added very similar test)

Copy link

@daniel-ge daniel-ge Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I was wrong I guess. Because of _copyfile_single_value a case like value = np.array([1,2,3]) and new_value = np.array([4,5,6]) will never happen, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@daniel-ge - sorry, but I've missed the main problem that the numpy arrays should not be here at all. I will review the code more carefully and post a new PR

setattr(result.output, field.name, new_value)
return result

Expand Down
16 changes: 16 additions & 0 deletions pydra/engine/tests/test_numpy_examples.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,22 @@ def arrayout(val):

def test_multiout(plugin):
""" testing a simple function that returns a numpy array"""
wf = Workflow("wf", input_spec=["val"], val=2)
wf.add(arrayout(name="mo", val=wf.lzin.val))

wf.set_output([("array", wf.mo.lzout.b)])

with Submitter(plugin=plugin, n_procs=2) as sub:
sub(runnable=wf)

results = wf.result(return_inputs=True)

assert results[0] == {"wf.val": 2}
assert np.array_equal(results[1].output.array, np.array([2, 2]))


def test_multiout_st(plugin):
""" testing a simple function that returns a numpy array, adding splitter"""
wf = Workflow("wf", input_spec=["val"], val=[0, 1, 2])
wf.add(arrayout(name="mo", val=wf.lzin.val))
wf.mo.split("val").combine("val")
Expand Down