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

Increase visibility on mismatch between dof ordering and node ordering #875

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

termi-official
Copy link
Member

Title checks out.

@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e806ed9) 93.27% compared to head (ee4cb84) 93.27%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #875   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files          36       36           
  Lines        5235     5235           
=======================================
  Hits         4883     4883           
  Misses        352      352           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Maximilian Köhler <maximilian.koehler@ruhr-uni-bochum.de>
@termi-official termi-official added the awaiting review PR is finished from the authors POV, waiting for feedback label Jan 15, 2024
Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Nice!
Some suggestions.

An alternative (or additional) solution if we have more of these questions could be to have a FAQ page that we can refer to.

Comment on lines +132 to +136
#md # ## What next?
#md # For more complicated visualization workflows we recommend either using our visualization tool [FerriteViz.jl](https://github.com/Ferrite-FEM/FerriteViz.jl)
#md # or users should export the solution into vtk files and use e.g. [ParaView](https://www.paraview.org/), [Mayavi](https://docs.enthought.com/mayavi/mayavi/), ... .
#md # It should be noted that the ordering of the DofHandler and the numbering of the nodes does not match, hence we cannot directly use solution
#md # vectors to assign colors to discretizations.
Copy link
Member

Choose a reason for hiding this comment

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

The part on VTK is already mentioned above. Would it be better to rename this subsection and write something like

Suggested change
#md # ## What next?
#md # For more complicated visualization workflows we recommend either using our visualization tool [FerriteViz.jl](https://github.com/Ferrite-FEM/FerriteViz.jl)
#md # or users should export the solution into vtk files and use e.g. [ParaView](https://www.paraview.org/), [Mayavi](https://docs.enthought.com/mayavi/mayavi/), ... .
#md # It should be noted that the ordering of the DofHandler and the numbering of the nodes does not match, hence we cannot directly use solution
#md # vectors to assign colors to discretizations.
#md # ## Visualizing grid data in Julia
#md # To visualize the solution on the grid, note that the numbering of the degrees of
#md # freedom generated by the `DofHandler` does not match the grid node ordering.
#md # To get data at the nodes, the function [`evaluate_at_grid_nodes`](@ref) can be used.
#md # For more complicated visualization workflows in Julia we recommend using our visualization
#md # tool [FerriteViz.jl](https://github.com/Ferrite-FEM/FerriteViz.jl)

(Haven't checked that if the links work)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. Above we only refer to the export of the fluxes, where we don't have a good way to compute them correctly in ParaView.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the regular export was already shown in the heat equation example that this how-to includes. In that case, perhaps it would make sense to modify that case above to say that one can then export the fluxes in addition to the temperature field, and modify that code block to

vtk_grid("heat_equation", grid) do vtk
    vtk_point_data(vtk, dh, u)
    vtk_point_data(vtk, projector, q_projected, "q")
end;

if that resolves that. But I think for new users the vtk export option is quite visible since it is used in most (all?) examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, but still users come to the conclusion that they better write their own visualizer over and over again, hence I wanted to really highlight that it is easier to use what is there already. :)

Copy link
Member

@KnutAM KnutAM Jan 16, 2024

Choose a reason for hiding this comment

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

OK, I've only seen that regarding doing visualization directly julia, and not regarding the vtk export?

Perhaps it would be nice to list the different options (e.g. vtk export, FerriteViz.jl, and roll your own (but then be warned about the dof ordering)) in the introduction of this how-to?

In a way, the L2projection is a more advanced topic than just visualizing the solution vector. So from that perspective, could it make sense to write a paragraph about options for that first, and then about projection and point eval?

docs/src/literate-tutorials/heat_equation.jl Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR is finished from the authors POV, waiting for feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants