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 option for higher order output #2851

Merged

Conversation

gassmoeller
Copy link
Member

@gassmoeller gassmoeller commented Mar 14, 2019

As discussed in #2841 this creates the option to write visualization data as higher order polynomials. It also adds a test that uses the option and a figure that shows the difference. First tests using this for some application models look good, and I think we should at some point enable this by default (likely when we require deal.II 9.1 at some point in the future).

Closes #2601.

Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

Hey Rene,
Very cool this! Just some small comments.

@@ -573,6 +573,10 @@ namespace aspect
vtk_flags.cycle = this->get_timestep_number();
vtk_flags.time = time_in_years_or_seconds;

#if DEAL_II_VERSION_GTE(9,1,0)
vtk_flags.write_higher_order_cells = write_higher_order_output;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be dependent on whether you set the option Write higher order output to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is. write_higher_order_output is only true if it is set in the input file, otherwise it is false and nothing changes.

Copy link
Member

Choose a reason for hiding this comment

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

ah, I now see. I overlooked it :(

New: There is now an option to output visualization data as higher order
polynomials. This is an improvement in accuracy and requires less disk space
than the 'Interpolate output' option that was available before (the new option
requires the old option to be set, which it is by default). However the new
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason you choose to make this option dependent on an other option? Can't we just deal with this internally?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but then setting Interpolate output would not change anything if Write higher order output is set. That might be confusing as well. Would you prefer the internal solution?

Copy link
Member

Choose a reason for hiding this comment

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

I Ithink the best option would be to have the user choose of the three options in one parameter, but that would not be backwards compatible.

I would personally prefer to have this done internally, because it is one thing less for the user to worry about/check, but I do not feel strongly about it and am also fine with leaving it this way. I will leave it up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case I prefer the current way 😄. In a few years the question will become irrelevant anyway, as we can then simply always use the higher order output if the interpolate option is set to true.
Maybe leave some time for others to comment (e.g. until tomorrow or so) and then feel free to merge.

@@ -462,6 +462,19 @@ namespace aspect
*/
bool filter_output;

/**
* deal.II offers the possibility to write vtu files with higher order
Copy link
Member

Choose a reason for hiding this comment

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

Capital D in Deal.II?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, it is usually deal.II, see https://dealii.org/. For reasons you would need to ask Wolfgang 😄. It looks a bit funny at the beginning of a sentence, but I think it is consistent with how it is usually used.

Copy link
Member

Choose a reason for hiding this comment

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

Since you where so consistent with it, I already suspected something like that. I just wanted to make sure :)

@MFraters
Copy link
Member

Should this wait for #2841, since you are stating that Interpolate output is on by default?

@gassmoeller
Copy link
Member Author

Probably yes, but #2841 is also ready from my side, so feel free to merge both if you feel comfortable with them.

@gassmoeller gassmoeller force-pushed the add_option_for_higher_order_output branch from f38ce3e to 64c109f Compare March 25, 2019 19:13
@gassmoeller
Copy link
Member Author

I rebased and resolved the conflict. Should be ready to go.

@MFraters MFraters merged commit 7aedd75 into geodynamics:master Mar 25, 2019
@gassmoeller gassmoeller deleted the add_option_for_higher_order_output branch March 26, 2019 17:00
freddrichards pushed a commit to freddrichards/aspect that referenced this pull request May 20, 2019
…igher_order_output

Add option for higher order output
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.

None yet

2 participants