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

Add fluid result averaging, FluidMeshInfo, and Faces #435

Merged
merged 43 commits into from
Jul 26, 2023

Conversation

PProfizi
Copy link
Contributor

@PProfizi PProfizi commented Jul 21, 2023

  • Mark TODO: filter-out zones from zone_ids and qualifiers different from requested location for integrated results
  • Mark TODO: Filter-out face zones from zone_ids or qualifiers when requesting results on cells
  • Mark TODO: Filter-out cell zones from zone_ids or qualifiers when requesting results on faces
  • Implement averaging with to_nodal_fc (cells to nodes and faces to nodes)
  • Implement averaging with to_elemental_fc (nodes to cells, nodes to faces)

Logic:
No averaging of integrated results anywhere else
No averaging between faces and cells
Averaging between nodes and cells
Averaging between faces and nodes

@PProfizi PProfizi added the enhancement New feature or request label Jul 21, 2023
@PProfizi PProfizi self-assigned this Jul 21, 2023
@PProfizi PProfizi added this to the v0.5.0 milestone Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #435 (4204663) into master (c9b5fc9) will decrease coverage by 0.11%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
- Coverage   83.49%   83.38%   -0.11%     
==========================================
  Files          44       45       +1     
  Lines        4640     4959     +319     
==========================================
+ Hits         3874     4135     +261     
- Misses        766      824      +58     

A ``result_type_enum`` value to inform a test on requested zones is necessary.
An integrated result will for example only be available on faces.
integrated:
An integrated result cannot be requested on another location than its native_location.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we mention the actual definition of integrated result? Integrated results in Dpf are indeed able to be averaged. It's Fluids+integrated results the ones that aren't (to be compliant with FBU's directives)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rafacanton could you give me a description of why FBU's directive is to not average integrated fluid results?

for every node at each element. Similarly, using `post.locations.elemental`
gives results with one value for each element, while using `post.locations.nodal`
gives results with one value for each node.
`post.locations.elemental`, and `post.locations.faces`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add post.locations.cell to comply with fluid vocabulary?
In the same topic, I'm wondering if we should have at least some docstrings mentioning "reconstruction" (the equivalent of "averaging" in fluid vocabulary) @rafacanton. It will help if people make a search in the doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbellot000 @rafacanton the issue is there is no gate.locations.cells, yet post.locations comes from core.locations which comes from gate.locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PProfizi @cbellot000 The issue with adding locations.cells in gate/pydpf-core is that they are the same thing as locations.Elemental, so I would be against adding them there.

# Raise for integrated result queried on anything else than its native location
if integrated:
if (native_location == "Faces" and location != locations.faces) or (
native_location == "Elements" and location != locations.elemental
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we not using the locations strings in here?

Copy link
Contributor Author

@PProfizi PProfizi Jul 24, 2023

Choose a reason for hiding this comment

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

@cbellot000 because the native_location strings in the available results are not the same as what you can find in locations

"""Returns a string representation of _FaceList object."""
return f"{self.__class__.__name__}({self}, __len__={len(self)})"

def _short_list(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that name is appropriate regarding what the function does...

@PProfizi PProfizi changed the title Add fluid result averaging Add fluid result averaging, FluidMeshInfo, and Faces Jul 26, 2023
@PProfizi PProfizi merged commit d726d15 into master Jul 26, 2023
26 checks passed
@PProfizi PProfizi deleted the feat/fluid_result_averaging branch July 26, 2023 15:31
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.

5 participants