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

Fix combination of skin extraction with scoping #673

Merged
merged 34 commits into from
Aug 28, 2024

Conversation

janvonrickenbach
Copy link
Contributor

@janvonrickenbach janvonrickenbach commented Jul 30, 2024

fixes #423

This PR fixes the following problem: The skin mesh operator stores the mapping of skin elements to solid elements as element indices (instead of element ids). The solid_to_skin_fc operator by default relies on these indices for the mapping of the solid data onto the skin mesh. This means that solid_to_skin_fc operator only works correctly if it is passed the same mesh that was the input for the skin operator. There is a second input to the solid_to_skin_fc operator which allows to pass the mesh on which the skin extraction happened (as opposed to the support of the result field). If I pass the correct mesh there, the data is correct for the model attached to the bug.

Todo:

  • Discuss why equivalent strains are computed before the averaging
  • Verify cyclic results

Some expectations have changed, but I believe the old results were incorrect. Here is an example results before the changes:

test_simulation py_TestHarmonicMechanicalSimulation_test_stress_skin (call)_

And this is the result with the fixes:
test_simulation py_TestHarmonicMechanicalSimulation_test_stress_skin (call)_skin_eqv

Here is the updated workflow:
workflow_0x0000019BC2ACAF20

and this was the old workflow:
workflow_0x000002E2E3A3A0B0

The two workflows are essentially the same, I just added the "skin_input_mesh" output. Also some forwarding operators were added to simplify the code.

I added extensive tests which uncovered the following issues (see more details in the tests). I would not address these issues in this PR as their resolution needs to be further discussed.

  1. The principal strains are computed incorrectly in the skin workflow, because the off-diagonal elements are not multiplied by 0.5. This depends on a "strain" field property which is not propagated correctly in the skin workflow. I guess this can be fixed by correctly forwarding the "strain" property in the skin operators. Note that this cannot be fixed in python since the field properties are not exposed. Fixed in backend (solid to skin operator now propagates the header).

  2. Skin extraction for elemental nodal data does currently not work. The workflow just returns the elemental nodal data of the solid in this case. Created issue Skin extraction for elemental nodal data does not work #685

  3. When comparing nodal results on the scoped skin to the corresponding data of the complete mesh, there are discrepancies at the boundaries. In the skin workflow the elemental nodal data is scoped and then averaged. This means that at the boundaries of the scope, only the elemental nodal data "inside" the scope is taken into account for the nodal average. => this is considered ok tests are adapted to reproduce this behavior

  4. Another discrepancy comes from the fact that the averaging happens on the skin mesh. This means that in corners the adjacent elements get different weights depending on the configuration. Consider the example below.
    The average of the circled node computed on the skin is the elemental nodal value of skin elements (black numbers 1,2,3,4,5) for this node. So the elements 1 and 3 (yellow numbers) are considered twice in the averaging, because they have two faces that touch the node. When averaging on the solids, the node would just be the average of the elemental nodal value of the three solid elements. => this is considered ok, tests are adapted to reproduce this behavior

  5. Skin extraction does not work out of the box if the result file contains different element types apart from solid elements. In this case the skin input needs to be rescoped to the solid elements. Created bug: Skin extraction with elemental averaging fails with mixed models #686

Both points 3) and 4) could be resolved by first averaging the data and then extracting the skin / element scope. However this would imply a performance penalty. We could also do the averaging on the external layer only. The values at the boundaries can only be correct if an additional layer of elements "around" the element scope is included in the averaging.

image

skin_operator = operators.mesh.skin(server=self._server)
self._selection.add_operator(skin_operator)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add forwarding operators so we can simplify the routing.

self._selection.set_input_name(
_WfNames.initial_mesh, mesh_provider_cyc, 100
) # hack
mesh_input = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why mesh_input was set to None here. It is checked on line 459, but I just replaced this checked with a not _is_model_cyclic check.

@@ -2071,6 +2093,7 @@ def test_stress_skin2(self, frame_modal_simulation: post.ModalMechanicalSimulati

def test_strain_skin(self, frame_modal_simulation: post.ModalMechanicalSimulation):
if frame_modal_simulation._model._server.meet_version("7.1"):
# todo I think this should be strain instead of stress
result = frame_modal_simulation.stress_principal_elemental(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBD: There are some tests labeled test_strain_skin, but they actually test stress results. I think this should be changed.

@janvonrickenbach janvonrickenbach marked this pull request as ready for review August 14, 2024 15:10
Comment on lines +128 to +129
# TBD: Why do we put the the equivalent operator
# before the averaging operator for strain results?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @janvonrickenbach,
If I remember correctly, it should be because this is what is done in the Mechanical workflow.

Copy link
Contributor Author

@janvonrickenbach janvonrickenbach Aug 27, 2024

Choose a reason for hiding this comment

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

Thanks, this is also what Camille mentioned. I will add a note in the upcomping refactoring PR.

@PProfizi
Copy link
Contributor

Hi @janvonrickenbach, this PR looks great to me! We do indeed need to create separate issues for what you uncovered. Thanks!

Copy link
Contributor

@FedericoNegri FedericoNegri left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this, the detailed explanation and follow ups on those other issues you found. I've installed your branch in our app and could verify that the original issue is fixed, it works as intended now.

While testing it, I did notice that it still takes an unexpectedly large amount of time to extract results on skin scoped to a named selection. On the same model I shared in the bug description, it takes <0.1s on the full mesh vs 4 seconds on the skin!

I looked into the dpf debug log and it seems like most of the time is spent in the by_scoping operator

image
which I believe we're calling in SpatialSelection.select_skin (the operator is called from_scoping in Python). (I already reported in a different context findings of the by_scopings operator being too slow.)

I know this is unrelated to this PR, but I'd like to get confirmation of the timings I get and double check that using that operator is the right approach. We can then follow up accordingly.

@janvonrickenbach
Copy link
Contributor Author

Thanks for addressing this, the detailed explanation and follow ups on those other issues you found. I've installed your branch in our app and could verify that the original issue is fixed, it works as intended now.

While testing it, I did notice that it still takes an unexpectedly large amount of time to extract results on skin scoped to a named selection. On the same model I shared in the bug description, it takes <0.1s on the full mesh vs 4 seconds on the skin!

I looked into the dpf debug log and it seems like most of the time is spent in the by_scoping operator

image which I believe we're calling in SpatialSelection.select_skin (the operator is called from_scoping in Python). (I already reported in a different context findings of the by_scopings operator being too slow.)

I know this is unrelated to this PR, but I'd like to get confirmation of the timings I get and double check that using that operator is the right approach. We can then follow up accordingly.

I can confirm that the by_scoping operator by far the bottleneck in the "skin with element subset" workflow. For me the skin workflow with all elements selected (skin=True) is very fast (<1 ms). However, getting the full result is also relatively slow (see below). I'm not sure why the "S" operator is slow while the "mapdl::rst:S" operator is does not take any time @PProfizi @cbellot000 Could you comment on these numbers and possible alternative solutions?

TIME REPORT
##########################################################################################################################

                                                 method name      number of call     total time (ms)    avg time by call
------------------------------------------------------------------------------------------------------------------------
                                                       :Id-1                   1                   0                   0
                                                      INPUTS                  10                   0                   0
                                                     OUTPUTS                  10                   0                   0
                                     ResultInfoProvider:Id19                   1                   0                   0
                                     ResultInfoProvider:Id21                   1                   0                   0
                                                      S:Id16                   1                3228                3228
                                           WORKFLOW RUN(out)                   1                3229                3229
                                      entity_average_fc:Id25                   1                   1                   1
                                                forward:Id10                   1                   0                   0
                         mapdl::rst::ResultInfoProvider:Id20                   1                   0                   0
                         mapdl::rst::ResultInfoProvider:Id22                   1                   0                   0
                                          mapdl::rst::S:Id23                   1                   1                   1
                                        to_elemental_fc:Id24                   1                   3                   3
##########################################################################################################################

@cbellot000
Copy link
Contributor

##################################################

On the top of my mind, I don't see workarounds. We need to investigate the performance issue on mesh by scoping(s), FYI @rafacanton

@janvonrickenbach
Copy link
Contributor Author

Thanks for addressing this, the detailed explanation and follow ups on those other issues you found. I've installed your branch in our app and could verify that the original issue is fixed, it works as intended now.
While testing it, I did notice that it still takes an unexpectedly large amount of time to extract results on skin scoped to a named selection. On the same model I shared in the bug description, it takes <0.1s on the full mesh vs 4 seconds on the skin!
I looked into the dpf debug log and it seems like most of the time is spent in the by_scoping operator
image which I believe we're calling in SpatialSelection.select_skin (the operator is called from_scoping in Python). (I already reported in a different context findings of the by_scopings operator being too slow.)
I know this is unrelated to this PR, but I'd like to get confirmation of the timings I get and double check that using that operator is the right approach. We can then follow up accordingly.

I can confirm that the by_scoping operator by far the bottleneck in the "skin with element subset" workflow. For me the skin workflow with all elements selected (skin=True) is very fast (<1 ms). However, getting the full result is also relatively slow (see below). I'm not sure why the "S" operator is slow while the "mapdl::rst:S" operator is does not take any time @PProfizi @cbellot000 Could you comment on these numbers and possible alternative solutions?

TIME REPORT
##########################################################################################################################

                                                 method name      number of call     total time (ms)    avg time by call
------------------------------------------------------------------------------------------------------------------------
                                                       :Id-1                   1                   0                   0
                                                      INPUTS                  10                   0                   0
                                                     OUTPUTS                  10                   0                   0
                                     ResultInfoProvider:Id19                   1                   0                   0
                                     ResultInfoProvider:Id21                   1                   0                   0
                                                      S:Id16                   1                3228                3228
                                           WORKFLOW RUN(out)                   1                3229                3229
                                      entity_average_fc:Id25                   1                   1                   1
                                                forward:Id10                   1                   0                   0
                         mapdl::rst::ResultInfoProvider:Id20                   1                   0                   0
                         mapdl::rst::ResultInfoProvider:Id22                   1                   0                   0
                                          mapdl::rst::S:Id23                   1                   1                   1
                                        to_elemental_fc:Id24                   1                   3                   3
##########################################################################################################################

For future reference: solid workflow was slow because of the license checkout.

@janvonrickenbach janvonrickenbach merged commit 507c1e5 into master Aug 28, 2024
30 checks passed
@janvonrickenbach janvonrickenbach deleted the jvonrick/fix_skin_extraction_with_selection branch August 28, 2024 15:52
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.

Issues combining mesh skin with scoping
4 participants