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

Enable output interpolation by default #2841

Merged

Conversation

Projects
None yet
5 participants
@gassmoeller
Copy link
Contributor

gassmoeller commented Mar 7, 2019

This is a change that is pretty significant although it is simply changing the default value of the Interpolate output setting of the visualization postprocessor from false to true.
Since #213 we had the option to let the visualization postprocessor write a mesh of higher resolution than the one we compute on. This counteracts the fact that Paraview and Visit usually only visualize linear functions but internally we usually have quadratic polynomials. Personally this is one of the settings I always change from the default for any computation I make, at least any time I want to look at the visualization output. It is essentially a free increase in output resolution.

Reasons to change the default:

  • I repeatedly met users who were not aware of the setting and were surprised by the improved output quality. While the numerical accuracy does not change, the output becomes more useful if we have a better representation of the results in Paraview.
  • If I always have to change a value that suggests the default is poorly chosen.
  • It better shows off the accuracy of ASPECT's solution.

Reasons to not change the default:

  • It could be confusing to have a different number of cells in the visualization output than in the screen output. This could be explained in the documentation of the visualization postprocessor and the manual.
  • The test resolutions need to be adjusted to not become too big (I am happy to do that if we decide this is a good idea).
  • There are a few exotic cases where you can get worse results (little oscillations or a mesh pattern in the output) with the setting activated. Namely if a visualization postprocessor (or material model) uses its surrounding cell midpoint or vertices to determine some value this point will be the midpoint of the cell of the triangulation (the coarse cell), not the cell of the output (the interpolated cell). This has happened to me in one model, since we introduced the setting.

I would like to hear opinions about this, I am open to discussion. I think activating it and taking advantage of the higher resolution makes it worth the occasional confusion. What do you think?

@naliboff

This comment has been minimized.

Copy link
Contributor

naliboff commented Mar 7, 2019

I would like to hear opinions about this, I am open to discussion. I think activating it and taking advantage of the higher resolution makes it worth the occasional confusion. What do you think?

I agree. It would be a shame for someone to accidentally or unknowingly not use this option when running a production model that benefits from the higher-resolution visualization.

@bangerth

This comment has been minimized.

Copy link
Contributor

bangerth commented Mar 8, 2019

Have you thought about the eight-fold increase in the amount of data and necessary disk space? You've been running some very large problems in the past and would have a better feel for how large these output files are, and if that's going to be a problem.

@bangerth

This comment has been minimized.

Copy link
Contributor

bangerth commented Mar 8, 2019

By the way, rather than adding a million lines to the output files, I'd much rather we add a couple lines to the input files in tests/ that just disable the additional output again.

@MFraters

This comment has been minimized.

Copy link
Contributor

MFraters commented Mar 8, 2019

I would like to hear opinions about this, I am open to discussion. I think activating it and taking advantage of the higher resolution makes it worth the occasional confusion. What do you think?

I have some mixed feelings about this, but I think that it is a good idea as long as can clearly define (or prevent the cases where it goes wrong.

I would be really exited about the higher order output for paraview into aspect, since it would add the extra detail without adding using too much extra disk space (which is a bit of an issue for me, and one of the reasons I currently always set it explicitly off, unless I want to inspect a problem) . Would it require a lot of work to get the higher order paraview files as an option in aspect?

@tjhei

This comment has been minimized.

Copy link
Member

tjhei commented Mar 8, 2019

By the way, rather than adding a million lines to the output files, I'd much rather we add a couple lines to the input files in tests/ that just disable the additional output again.

Agreed. That was my first thought when I saw this PR.

@tjhei

This comment has been minimized.

Copy link
Member

tjhei commented Mar 8, 2019

What do you think?

There are pros and cons for sure, but I am okay with changing the default.

@gassmoeller

This comment has been minimized.

Copy link
Contributor Author

gassmoeller commented Mar 13, 2019

Sorry for the gap in replies, I was at a workshop. Yes you are right the tests need adjustment, I will get to that later this week.

About the increase in output files: It is true that the files become larger, however I see it not as a disadvantage. For applied scientific studies this increase is nearly equivalent to running at a higher resolution, which would also require bigger output files. I am not so much concerned about model accuracy per output GB and more with usable output information per computation time.

Btw: I also tested the higher order output and it seems to work with Paraview 5.5. Separate PR incoming. I still think this is useful separately (without the output interpolation the higher order output is not active anyway, and for hdf5 users the interpolation is also useful).

@gassmoeller gassmoeller force-pushed the gassmoeller:enable_output_interpolation_by_default branch from eec124d to f8bbc4d Mar 14, 2019

@gassmoeller gassmoeller changed the title [Discussion] Enable output interpolation by default Enable output interpolation by default Mar 14, 2019

@gassmoeller gassmoeller force-pushed the gassmoeller:enable_output_interpolation_by_default branch from d6cab7b to d9ce6d7 Mar 14, 2019

@gassmoeller

This comment has been minimized.

Copy link
Contributor Author

gassmoeller commented Mar 14, 2019

Ok, I updated the tests to generally not use the feature. A few tests enable it explicitly, I left them that way so there are tests to make sure it works. Ready from my side.

@gassmoeller gassmoeller force-pushed the gassmoeller:enable_output_interpolation_by_default branch from d9ce6d7 to 768f553 Mar 18, 2019

@gassmoeller gassmoeller force-pushed the gassmoeller:enable_output_interpolation_by_default branch from 768f553 to cf66c82 Mar 18, 2019

@gassmoeller

This comment has been minimized.

Copy link
Contributor Author

gassmoeller commented Mar 22, 2019

So did we conclude we want to go forward with this PR? From my side it is ready, and I would like to see it merged, because #2851 depends on it.

@tjhei

tjhei approved these changes Mar 23, 2019

@tjhei

This comment has been minimized.

Copy link
Member

tjhei commented Mar 23, 2019

I think this is good to merge. Any objections?

@MFraters
Copy link
Contributor

MFraters left a comment

Looks good to me :)

@MFraters MFraters merged commit 363a216 into geodynamics:master Mar 24, 2019

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins.tjhei.info/pr-head This commit looks good
Details

@gassmoeller gassmoeller deleted the gassmoeller:enable_output_interpolation_by_default branch Mar 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.