Skip to content

Conversation

FedericoNegri
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #543 (87470d9) into master (6f656d4) will increase coverage by 0.04%.
The diff coverage is 91.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
+ Coverage   83.82%   83.86%   +0.04%     
==========================================
  Files          47       47              
  Lines        5068     5083      +15     
==========================================
+ Hits         4248     4263      +15     
  Misses        820      820              

comp = None
base_name += "_N"

return wf, out, selection
Copy link
Contributor

@PProfizi PProfizi Nov 16, 2023

Choose a reason for hiding this comment

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

@FedericoNegri why do you need to define the output of the workflow (set_output_name) later and not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PProfizi In a different context, I'm using the _get_result_workflow function to get the workflow and chain additional operators, for instance:

wf, out = _get_result_workflow(...)

# add min/max filters
min_max_op = simulation._model.operator(name="min_max_fc")
min_max_op.connect(0, out)
wf.add_operator(operator=min_max_op)

# Set the workflow output
wf.set_output_name("min", min_max_op.outputs.field_min)
wf.set_output_name("max", min_max_op.outputs.field_max)

Do you suggest doing differently?

Copy link
Contributor

@PProfizi PProfizi Nov 16, 2023

Choose a reason for hiding this comment

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

@FedericoNegri @cbellot000 I've had the same sort of issues while trying to make dynamic workflows, I think what we need is to add getter to Workflow, such as Workflow.get_output_by_name which returns the input/ouput as a proper Operator.Input or Operator.Output the same way we can set_output_name or set_input_name, otherwise we keep having to pass a variable to not loose it. The alternative is to use Workflow.connect_with where you gather what you want to add in a second worklow. In your case though it may be exactly what you need. This is for example what we do here where we create a mesh extraction workflow which we connect to the result extraction workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PProfizi thanks for the feedback. I can try out the connect_with route, but yeah ideally the first approach would be more convenient. Anyways, I've reworked the _get_result_workflow so that now it only returns the workflow. Please have a look.

@FedericoNegri FedericoNegri marked this pull request as ready for review November 27, 2023 13:20
@FedericoNegri FedericoNegri changed the title [WIP] Expose workflow to extract results from simulation Expose workflow to extract results from simulation Nov 27, 2023
@FedericoNegri FedericoNegri merged commit 033bb9a into master Nov 28, 2023
@FedericoNegri FedericoNegri deleted the fnegri/expose_result_workflow branch November 28, 2023 13:23
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.

3 participants