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

[feat] Support post processing predict as learnt and predict on cells for surface variables #117

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yias
Copy link
Contributor

@yias yias commented Dec 24, 2024

Description

This PR enables users to trigger post-processing on surface variables with location options; on cells or as learnt.

To manage the selection of endpoints for the two options, the classes PostProcessingOnCells and PostProcessingAsLearnt are introduced as child-classes of SurfaceVTP. These choices are enumerated in PPSurfaceLocation. The methods PredictionPostProcessings.surface_vtp and SelectionPostProcessingsMethods.surface_vtp have now the argument pp_location, which should be one enumeration option of PPSurfaceLocation. The default option is set to

Story: [sc-26496]

Tests

  • Unit-tests added
  • Interactive smoke testing against dev env.

PS. It requires merging the corresponding PR of the API support.

@yias yias self-assigned this Dec 24, 2024
@yias yias requested a review from a team as a code owner December 24, 2024 15:38
@yias yias requested review from citronella3alain and awoimbee and removed request for a team December 24, 2024 15:38
@yias yias added the enhancement New features or code improvements label Dec 24, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 24, 2024
@yias yias requested a review from tmpbeing December 24, 2024 15:57
Copy link
Collaborator

@tmpbeing tmpbeing left a comment

Choose a reason for hiding this comment

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

I think class names need to change. And we might want to turn SurfaceVTP into _SurfaceVTP and undocument it if it's not meant to be used directly anymore. Hell maybe remove it entirely given the class is empty.

@@ -56,6 +56,14 @@ Available postprocessings
:members:


.. autoclass:: PostProcessingOnCells()
:members:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to have inherited-members as well.

@@ -274,6 +275,40 @@ class SurfaceVTP(_PostProcessingVTKExport):
"""


class PostProcessingOnCells(SurfaceVTP):
Copy link
Collaborator

@tmpbeing tmpbeing Dec 26, 2024

Choose a reason for hiding this comment

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

I didn't follow the product story but these class name seems too generic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants