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

Specific times in output #2079

Merged
merged 10 commits into from Mar 1, 2018

Conversation

Projects
None yet
3 participants
@wenqing
Copy link
Member

wenqing commented Feb 15, 2018

This PR adds a functionality to make output of results at given times. For this purpose, an optional input of specified times is introduced to createOutput, with which the specified times for output can be given in project file. For example

        <output>
             <type>VTK</type>
            <prefix>richards</prefix>
            <timesteps>
                <pair>
                    <repeat>1</repeat>
                    <each_steps>100000000</each_steps>
                </pair>
            </timesteps>
            <specified_times> 50.0 100.0 500.</specified_times>
            <output_iteration_results>false</output_iteration_results>
        </output>

For computation, the specified times given in the output data are merged to the specified times in the time stepper. One can input the specified times in any order or even with duplicated data (e.g. by mistakes), however their order is sorted in the descending way and the data are made unique after input. In the computation, the time stepping is conducted at every specified time. So far, the functionality is for the adaptive time stepper, EvolutionaryPIDcontroller.

Test Parabolic/Richards/RichardsFlow_2d_small_adaptive_dt.prj is modified to test the presented functionality.

@@ -91,6 +91,10 @@ class EvolutionaryPIDcontroller final : public TimeStepAlgorithm
/// Get a flag to indicate that this algorithm need to compute
/// solution error.
bool isSolutionErrorComputationNeeded() override { return true; }
/// Add specific times
void addSpecificTimes(

This comment has been minimized.

@endJunction

endJunction Feb 15, 2018

Member

It would be better to add comment on what this function does aside the info given in the function's name.

This comment has been minimized.

@wenqing

wenqing Feb 16, 2018

Author Member

Added a more detailed description.

@@ -85,6 +85,12 @@ class TimeStepAlgorithm
/// Get a flag to indicate whether this algorithm needs to compute
/// solution error. The default return value is false.
virtual bool isSolutionErrorComputationNeeded() { return false; }

/// Add specific times
virtual void addSpecificTimes(std::vector<double> const& /*specific_times*/)

This comment has been minimized.

@endJunction

endJunction Feb 15, 2018

Member

Actually the general description would be here, and the overriding function in PID controller would provide additional info...

This comment has been minimized.

@wenqing

wenqing Feb 16, 2018

Author Member

Use \copydoc

return make_output;

const double specific_time = _specific_times.back();
const double zero_threshold = std::numeric_limits<double>::epsilon();

This comment has been minimized.

@endJunction

endJunction Feb 15, 2018

Member

epsilon is not zero. :-)

This comment has been minimized.

@wenqing

wenqing Feb 16, 2018

Author Member

Changed to min, while changed the tolerance for time step size calculation with specified times.

Output::Output(std::string output_directory, std::string prefix,
bool const compress_output, std::string const& data_mode,
bool const output_nonlinear_iteration_results,
std::vector<PairRepeatEachSteps> repeats_each_steps)
std::vector<PairRepeatEachSteps> repeats_each_steps,
const std::vector<double>&& specific_times)

This comment has been minimized.

@endJunction

endJunction Feb 15, 2018

Member

Just curious, why the const r-value?

This comment has been minimized.

@wenqing

wenqing Feb 16, 2018

Author Member

removed const from there. Just wonder, the code can be compiled with that const.

@@ -109,6 +109,7 @@
<each_steps>100000000</each_steps>
</pair>
</timesteps>
<specific_times> 50.0 100.0 500.</specific_times>

This comment has been minimized.

@endJunction

endJunction Feb 15, 2018

Member

I'm not sure about the "specific" here. Just my opinion, other comments are welcome.
I'd rather call this fixed_output_times, because for me, the "specific" alone does not necessary mean "output" in this context.

This comment has been minimized.

@wenqing

wenqing Feb 16, 2018

Author Member

Changed the name to specified times.

@wenqing wenqing force-pushed the wenqing:specific_time_in_output branch 2 times, most recently from 304100f to 0995620 Feb 16, 2018

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Feb 19, 2018

Other ideas additionally to fixed_output_times mentioned above would be forced_output, additional_timesteps, and additional_output.

Suggestions are welcome!

specified is much better, than specific, but the other output times are also "specified", so it could be more clearly named....

@Thomas-TK

This comment has been minimized.

Copy link
Member

Thomas-TK commented Feb 28, 2018

Concerning animations and tool coupling we need two things:
a) a forced output at user-defined specific times
b) an output with regular time step sizes (e.g. every hour), but this can be also done by generating lists and using a) I guess.
Therefore "fixed_output_times" sounds most intuitive to me.

@wenqing

This comment has been minimized.

Copy link
Member Author

wenqing commented Feb 28, 2018

Any other suggestion about the tag name? If not, fixed_output_times will be taken.

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Mar 1, 2018

@wenqing Take fixed_output_times, rebase, and merge when green.

@wenqing wenqing force-pushed the wenqing:specific_time_in_output branch from 0995620 to fc4de95 Mar 1, 2018

@endJunction endJunction merged commit 51db1bc into ufz:master Mar 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
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.