Specifying multiple particle output formats #1049

hlokavarapu · 2016-06-29T10:50:43Z

Using the container vector of output::interfaces, can now generate the output of multiple formats that are specified by the parameter "Data output format."

As per edge cases, if user specifies:
set Data output format = ascii, hdf5, none
then, none is honored.

If user specifies:
set Data output format = ascii, ascii, vtu, vtu
then, ascii and vtu data are generated only once.

bangerth · 2016-06-29T18:05:24Z

source/particle/world.cc

+
+      for ( typename std::vector<std_cxx11::shared_ptr<Output::Interface<dim> > >::const_iterator itr= output.begin(); itr != output.end(); itr++)
+        {
+          filename = (itr->get()->output_particle_data(particles,


do you need the ->get() part of this? I think you could get away by just saying itr->output_particle_data.

Also just remove the outer parentheses around the entire call.

On a separate note, you overwrite the filename variable each time. How about creating a list of filenames that, with comma separators, are ultimately going to be returned.

Since output is a vector of shared pointers, we dereference once to a shared pointer to output interface, and must dereference again to access the output interface object. itr->get()-> is necessary.

Ah, I see. I had thought that the compiler just inserts deference operations until it actually gets to an object without such an operator. I may misremember.

I misremembered.

bangerth · 2016-06-29T18:11:16Z

/run-tests

bangerth · 2016-06-29T18:11:39Z

Could you clone one of the existing tests that output particle information so that it writes in two or more formats?

gassmoeller · 2016-07-01T05:10:06Z

source/particle/world.cc

@@ -85,19 +85,26 @@ namespace aspect
    std::string
    World<dim>::generate_output() const
    {
+      std::vector<std::string> filenames;


What do you use this vector for? Do you intend to return all file names? But what to do with them?

Sorry, this was a leftover fragment of code. I originally intended to return a vector of file names but later realized that there wasn;t an immediate and clean way of writing these to the statistics file.

bangerth · 2016-07-12T11:47:58Z

Let us know when you have updated the patch!

hlokavarapu · 2016-09-13T22:06:39Z

tests/particle_output_multiple_formats/screen-output

@@ -16,40 +11,20 @@ Number of degrees of freedom: 4,645 (2,178+289+1,089+1,089)
   Solving Stokes system... 30+3 iterations.

   Postprocessing:
-     Writing particle output: output/particles/particles-00001.{txt,vtu,hdf5}
+     Writing particle output: output-particle_output_multiple_formats/particles/particles-00000.{txt,h5,vtu}


This is the output format that I have currently implemented, particles-00000.{txt,h5,vtu}.
If only one format is specified, for example ascii, the string is set to particles-00000.txt.
Is this clear cut?

hlokavarapu · 2016-09-13T22:35:26Z

Reviving a two month old PR that I have recently updated in order to touch base on the syntax for the particle output logging messages. Thanks @gassmoeller, @bangerth, @egpuckett. If we can finalize on the syntax, I can then proceed to update tests that are using particles.

hlokavarapu · 2017-05-14T18:22:51Z

@gassmoeller , this PR is ready to be reviewed!

bangerth

I'm not a big fan of the repetitiousness of this patch. Each particle output generator gets functions get_file_name(), get_particle_output_location(), and get_file_index(), but I think all of this could actually be handled in the base class if only a derived class could provide a file suffix (via an abstract function in the base class that derived classes have to overload -- or maybe more elegantly through an argument that derived classes pass to the constructor of the base class).

I think such a change would save a couple of hundred lines of repetitive code already.

hlokavarapu · 2017-05-16T16:41:42Z

PR is ready to be reviewed once more, (only took two days to get the abstraction correct) 😄

hlokavarapu · 2017-05-16T18:59:26Z

Previously, this PR would trigger the tester, however, this is no longer the case now... Why is this?

bangerth · 2017-05-16T19:15:17Z

/run-tests

bangerth

More comments coming, but this for now.

bangerth · 2017-05-16T19:24:43Z

include/aspect/particle/output/hdf5.h

           */
-          unsigned int file_index;
+          const std::string
+          get_file_name ();


It makes no difference whether the string you return here is const or not. const only makes a difference for reference and pointer types when returned from a function. Can you remove the qualification?

Also, since the function is virtual (because it is virtual in the base class), please mark it as virtual.

Fair point, fixed.

bangerth · 2017-05-16T19:25:00Z

include/aspect/particle/output/interface.h

+           */
+          virtual
+          const std::string
+          get_file_name ();


what does the default implementation of this class do? If the base class doesn't know what to do, then mark the function =0.

Also, is the function supposed to change member variables? If not, please mark the function const.

The default implementation is

template <int dim> std::string Interface<dim>::get_file_name () { return "particles-" + get_file_index() + "." + Utilities::int_to_string(Utilities::MPI::this_mpi_process(this->get_mpi_communicator()), 4) + output_file_suffix; }

Right, if the return type is not a pointer, then it shouldn;t be of type const but if the function itself doesn;t modify any provate or protected variables, then the function is of type const.

Right, but then you need to write the declaration as

virtual std::string get_filename() const;

Are any of the derived classes going to actually implement a different behavior than the base class?

Also, please document the default behavior of the function.

HDF5Output class does override this function as

template <int dim> std::string HDF5Output<dim>::get_file_name () const { return "particles-" + Interface<dim>::get_file_index () + this->output_file_suffix; }

mainly because at the moment, when running with multiple MPI processes, a single output file is written as if grouped files is set to 1. This isn;t the case for txt and vtu based output.

bangerth

More comments.

bangerth · 2017-05-16T22:22:44Z

source/particle/output/ascii.cc


-        return output_file_prefix;
+        return "txt";


This doesn't make much sense. The text is posted on the screen, so the real file name would be a useful thing to have. Or do you collate it later on from all output processors? If the latter, can you update the documentation of this function (in the base class) to explain what they are supposed to return? Or get the suffix from the base class via a new member function, given that the base class already knows the suffix?

I also am not fond of the return string but in essence, the return stings are being collected and formatted to produce a nice readable string, ie.

*** Timestep 0: t=0 seconds Skipping temperature solve because RHS is zero. Solving C_1 system ... 0 iterations. Rebuilding Stokes preconditioner... Solving Stokes system... 34+0 iterations. Postprocessing: Writing particle output: output/particles/particles-00000.{txt,vtu,h5}

I have updated the documentation in all of the header files in include/aspect/particle/output directory. Primarily, I specify that the return type of the function

virtual std::string output_particle_data(const std::multimap<types::LevelInd, Particle<dim> > &particles, const Property::ParticlePropertyInformation &property_information, const double current_time);

is a string that contains the file extension, i.e.:

* @return The name of the file extension that was written, or any other * information that describes what output was produced if for example * multiple files were created. */

OK, that works.

bangerth · 2017-05-16T22:23:37Z

source/particle/output/ascii.cc

-        ;
-      }
+      void ASCIIOutput<dim>::serialize (Archive &, const unsigned int)
+      {}


If some of the derived classes don't need to implement anything, then maybe move this function into the base class and only overload it in the derived classes if they need to do anything non-standard?

I'm going to note that this is now actively incorrect because you don't serialize the member variables of the base class.

I have moved the the function serialize into the base class, interface, and declared it as a virtual. Serializing the member variables, i.e:

template <int dim> template <class Archive> void Interface<dim>::serialize (Archive &ar, const unsigned int) { ar &output_file_suffix &file_index ; }

In terms of serialization, is it a top down model in that the parent classes serialize function is called followed by the serialize function of the childs class?

I am planning on running the debugger to get to the bottom of this question.

You can't make template functions virtual. You can only declare a function of the same name in a derived class, i.e., have the same template function in the derived class as in the base class. The function in the derived class would then serialize its own variables and then call the corresponding function in the base class that would serialize the variables in the base class (or do it the other way around).

bangerth · 2017-05-16T22:23:56Z

source/particle/output/hdf5.cc

      template <int dim>
      HDF5Output<dim>::HDF5Output()
        :
-        file_index(0)
+        Interface<dim> ()


No need for this line -- it is implied.

bangerth · 2017-05-16T22:25:22Z

source/particle/output/interface.cc

@@ -44,23 +46,68 @@ namespace aspect
      template <int dim>
      void
      Interface<dim>::initialize ()
-      {}
+      {
+        file_index = 0;


move this into the member initializer list of the constructor. initialize() would then just be an empty function

bangerth · 2017-05-16T22:27:17Z

source/particle/output/interface.cc


+      template <int dim>


add two more empty lines here

bangerth · 2017-05-16T22:27:39Z

source/particle/output/vtu.cc

@@ -32,14 +32,15 @@ namespace aspect
      template <int dim>
      VTUOutput<dim>::VTUOutput()
        :
-        file_index(0)
+        Interface<dim> ()
      {}


Just remove the whole constructor.

bangerth · 2017-05-16T22:29:42Z

source/particle/world.cc

+
+      for ( typename std::vector<std_cxx11::shared_ptr<Output::Interface<dim> > >::const_iterator itr= output.begin(); itr != output.end(); itr++)
+        {
+          output_suffixes.push_back((itr->get()->output_particle_data(particles,


there is an unnecessary pair of parentheses after push_back

bangerth · 2017-05-16T22:31:12Z

source/particle/world.cc

-      output->save(os);
+
+      for ( typename std::vector<std_cxx11::shared_ptr<Output::Interface<dim> > >::const_iterator itr= output.begin(); itr != output.end(); itr++)
+        itr->get()->save(os);


this, and the corresponding load function only works if the list of output formats is exactly the same in the original run as in a restarted run. This is too brittle. Please copy the corresponding code from postprocess/interface.cc.

bangerth · 2017-05-16T22:31:59Z

source/particle/world.cc

-      // a null pointer. Only initialize output if it was created.
-      if (output)
+      // Remove duplicate output formats specified in the parameter file.
+      auto last = std::unique(output_format_names.begin(), output_format_names.end());


we can't do C++11 yet. You need to provide the correct type for last.

bangerth · 2017-05-16T22:32:34Z

source/particle/world.cc

+      if (output_format_names.size() != 1 || output_format_names[0] != "none")
+        for (std::vector<std::string>::const_iterator itr = output_format_names.begin();
+             itr != output_format_names.end(); itr++)
+          output.push_back(std_cxx11::shared_ptr<Output::Interface<dim>>


You need a space here, > >.

hlokavarapu · 2017-05-17T01:50:15Z

I have resolved all of your comments and I believe that this PR is ready to be reviewed one more time. However, I will continue to develop past the hackathon and am perfectly alright to delay the review process till next week. However given my track record. this might very well be the next hackathon. 😄

hlokavarapu · 2017-05-17T02:11:47Z

I have found a bug related to resuming snapshots. This is WIP.

hlokavarapu · 2017-05-17T03:17:10Z

Found and fixed the bug, so long as the tester is happy, I think this PR is ready to be reviewed.

gassmoeller

Looks generally ok. There are a few pitfalls in the implementation, please take a look at my comments. Nothing looks impossible to resolve, but we need to discuss that a bit.

gassmoeller · 2017-05-23T19:46:28Z

include/aspect/particle/output/ascii.h

@@ -81,31 +75,27 @@ namespace aspect
                               const Property::ParticlePropertyInformation &property_information,
                               const double current_time);

+


Please remove this empty line

gassmoeller · 2017-05-23T19:47:08Z

include/aspect/particle/output/ascii.h

-           * Save the state of the object.
-           */
+          * Save the state of the object.
+          */


Please revert this comment change, I think the old version was our usual convection.

gassmoeller · 2017-05-23T19:48:21Z

include/aspect/particle/output/ascii.h

@@ -37,16 +37,10 @@ namespace aspect
       * @ingroup ParticleOutput
       */
      template <int dim>
-      class ASCIIOutput : public Interface<dim>,
-        public SimulatorAccess<dim>
+      class ASCIIOutput : public Interface<dim>


Please remove the inclusion of simulator_access.h at the beginning of the header file, if you no longer (directly) derive this class from SimulatorAccess. Same for all other plugins that you modified in this PR.

gassmoeller · 2017-05-23T19:50:29Z

include/aspect/particle/output/interface.h

@@ -40,10 +40,17 @@ namespace aspect
       * @ingroup ParticleOutput
       */
      template <int dim>
-      class Interface
+      class Interface : public SimulatorAccess<dim>


You need to add simulator_access.h in the list of includes at the top of this file. It currently seems to work without, because one of the other includes includes that one implicitly, but we can not rely on that fact.

gassmoeller · 2017-05-23T19:50:47Z

include/aspect/particle/output/interface.h

+           */
+          Interface ()
+            :
+            file_index (0) {}


Please move the definition of this constructor into the .cc file.

gassmoeller · 2017-05-23T20:12:02Z

source/particle/world.cc

+      const std::string output_directory = output[0].get()->get_particle_output_location();
+      const std::string file_index = output[0].get()->get_file_index();
+
+      for ( typename std::vector<std_cxx11::shared_ptr<Output::Interface<dim> > >::const_iterator itr= output.begin(); itr != output.end(); itr++)


use ++itr unless there is need for itr++ (almost never, unless you modify the container within the loop). If you are interested in the topic here is a somewhat lengthy discussion: https://stackoverflow.com/questions/484462/difference-between-i-and-i-in-a-loop?noredirect=1&lq=1).

Also, we usually give iterators explanatory names, in this case output_object comes to mind. While it is longer, it makes the following lines easier to understand.

gassmoeller · 2017-05-23T20:17:37Z

source/particle/world.cc

+                                output_format_names.end());
+
+      // Create an output object depending on what the parameters specify so long as none has not been specified.
+      // If none and more than one other format is specified, then we ignore none.


Do you mean: "If none and one or more other format is specified, ..." ?

gassmoeller · 2017-05-23T20:25:17Z

source/particle/world.cc

+                   ExcMessage ("If you specify 'none' for the output format for parameter \"Data output format\","
+                               "then this needs to be the only value given."));
+
+      if (output_format_names.size() != 1 || output_format_names[0] != "none")


After the AssertThrow above we are sure that if "none" was selected the output_format_names vector has size 1. So you could add the following line:

if (output_format_names[0] == "none") output_format_names.clear();

This would allow you to simplify the logic in the current line to if (output_format_names.size() > 0) and include the other loop below into this if-loop. I feel that would be easier to understand.

gassmoeller · 2017-05-23T20:30:15Z

source/particle/world.cc

+
+      if (output_format_names.size() != 1 || output_format_names[0] != "none")
+        for (std::vector<std::string>::const_iterator itr = output_format_names.begin();
+             itr != output_format_names.end(); itr++)


Again please rename the iterators in the two for-loops. output_format and output_object come to mind.

gassmoeller · 2017-05-23T20:39:51Z

source/particle/world.cc


-      return filename;
+      const std::string output_directory = output[0].get()->get_particle_output_location();
+      const std::string file_index = output[0].get()->get_file_index();


Here you assume that file_index is the same for all output plugins. However, inside of the serialization of the plugins you seem to allow that they can be enabled or disabled during restarts (leading to different file indices in the different plugins). Would this lead to problems? At least it would cause the screen output and statistics output of the file names to be not consistent with the actual filenames of some plugins. Is there an easy fix for that? If not, how can we best document this behavior?

bangerth · 2017-08-04T12:36:28Z

Let's close this given that #1869 is around now. I think re-implementing things in that framework would be a more useful approach, as discussed there.

bangerth reviewed Jun 29, 2016
View reviewed changes

gassmoeller reviewed Jul 1, 2016
View reviewed changes

hlokavarapu reviewed Sep 13, 2016
View reviewed changes

hlokavarapu force-pushed the Multiple_particle_output branch 2 times, most recently from 963deed to 8d734e1 Compare May 14, 2017 17:47

bangerth reviewed May 14, 2017

View reviewed changes

hlokavarapu force-pushed the Multiple_particle_output branch 3 times, most recently from c85ba34 to 204b49c Compare May 16, 2017 16:40

bangerth reviewed May 16, 2017

View reviewed changes

bangerth requested changes May 16, 2017

View reviewed changes

hlokavarapu changed the title ~~Specifying multiple particle output formats~~ [WIP] Specifying multiple particle output formats May 17, 2017

Add support for multiple particle output formats

9ed1485

hlokavarapu force-pushed the Multiple_particle_output branch from 07199b8 to 9ed1485 Compare May 17, 2017 03:16

hlokavarapu changed the title ~~[WIP] Specifying multiple particle output formats~~ Specifying multiple particle output formats May 17, 2017

gassmoeller reviewed May 23, 2017

View reviewed changes

gassmoeller mentioned this pull request Jul 26, 2017

Rewrite particle output to use deal.II functionality #1869

Merged

2 tasks

bangerth closed this Aug 4, 2017

		@@ -81,31 +75,27 @@ namespace aspect
		const Property::ParticlePropertyInformation &property_information,
		const double current_time);

Specifying multiple particle output formats #1049

Specifying multiple particle output formats #1049

Conversation

hlokavarapu commented Jun 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangerth commented Jun 29, 2016

bangerth commented Jun 29, 2016

Choose a reason for hiding this comment

hlokavarapu Jul 6, 2016 • edited

Choose a reason for hiding this comment

bangerth commented Jul 12, 2016

Choose a reason for hiding this comment

hlokavarapu commented Sep 13, 2016

hlokavarapu commented May 14, 2017

bangerth left a comment

Choose a reason for hiding this comment

hlokavarapu commented May 16, 2017

hlokavarapu commented May 16, 2017 • edited

bangerth commented May 16, 2017

bangerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlokavarapu May 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlokavarapu commented May 17, 2017 • edited

hlokavarapu commented May 17, 2017

hlokavarapu commented May 17, 2017

gassmoeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangerth commented Aug 4, 2017

hlokavarapu Jul 6, 2016 •

edited

hlokavarapu commented May 16, 2017 •

edited

hlokavarapu May 17, 2017 •

edited

hlokavarapu commented May 17, 2017 •

edited