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

Incremental workflow evaluation #946

Merged
merged 25 commits into from
Aug 1, 2023
Merged

Conversation

ansys-akarcher
Copy link
Contributor

@ansys-akarcher ansys-akarcher commented May 11, 2023

These changes brings a simple API around the for_each and chunk_in_for_each_range operators.

The goal is to be able to process very huge files with minimal effort, including being able to let the API choose for ourselves the chunk_size.

Also, this draft PR needs the changes from this one beforehand

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #946 (92b2985) into master (3223ca5) will increase coverage by 1.23%.
Report is 2 commits behind head on master.
The diff coverage is 84.40%.

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   86.35%   87.58%   +1.23%     
==========================================
  Files          80       81       +1     
  Lines        9078     9183     +105     
==========================================
+ Hits         7839     8043     +204     
+ Misses       1239     1140      -99     

examples/04-advanced/14-incremental_evaluation.py Outdated Show resolved Hide resolved
result_op.connect(4, ds)
new_end_op = splitter.split(chunk_size=2, scoping=scoping)

min2 = new_end_op.get_output(0, dpf.types.field)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does new_end_op have any generated outputs (to do new_end_op.outputs....()) ?

examples/04-advanced/14-incremental_evaluation.py Outdated Show resolved Hide resolved
examples/04-advanced/14-incremental_evaluation.py Outdated Show resolved Hide resolved
examples/04-advanced/14-incremental_evaluation.py Outdated Show resolved Hide resolved
@ansys-akarcher ansys-akarcher marked this pull request as ready for review June 14, 2023 08:39
Comment on lines 30 to 31
# last operator should support incremental evaluation
# but it should be permissive too (bad doc/spec whatever)
Copy link
Contributor

Choose a reason for hiding this comment

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

(for bad docs or specs?) Not sure what the content in this bracket means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was unclear.

Basically this function is used for validation, but in the case this function doesn't validate, it should not create an error but simply inform the user through a warning.
Because the operator that it is trying to validate may have a bad specification or documentation or whatever the inconsistency is.

Please tell me if the current comments are clearer in that regard.

@PProfizi
Copy link
Contributor

Hi @ansys-akarcher,
Is this PR still relevant? Or can we close it as stale?

@PProfizi PProfizi closed this Jul 25, 2023
@PProfizi PProfizi reopened this Jul 25, 2023
Copy link
Contributor

@cbellot000 cbellot000 left a comment

Choose a reason for hiding this comment

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

Approving, but split_workflow_in_chunks and IncrementalHelper class would benefit from more detailed docstrings explaining the parameters and what the class is doing

src/ansys/dpf/core/incremental.py Outdated Show resolved Hide resolved
@ansys-akarcher
Copy link
Contributor Author

ansys-akarcher commented Jul 26, 2023

Yes, I need to:

  • change a bit the tests
  • Update docstrings (and make them detailed as you do)



# Obtain results on the same pin numbers
min = new_end_op.get_output(0, dpf.types.field)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ansys-akarcher same remark as @cbellot000:
can't we write
min = new_end_op.outputs.min_field.eval()
or something similar?

Co-authored-by: Camille Bellot <80476446+cbellot000@users.noreply.github.com>
@PProfizi PProfizi added the enhancement New feature or request label Aug 1, 2023
@PProfizi PProfizi changed the title Feat/incremental eval Incremental workflow evaluation Aug 1, 2023
@ansys-akarcher ansys-akarcher merged commit 434fbe2 into master Aug 1, 2023
30 of 31 checks passed
@ansys-akarcher ansys-akarcher deleted the feat/incremental_eval branch August 1, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants