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

Rewrite particle output to use deal.II functionality #1869

Merged
merged 7 commits into from Aug 8, 2017

Conversation

gassmoeller
Copy link
Member

@gassmoeller gassmoeller commented Jul 26, 2017

This PR uses the recently introduced funtionality of deal.II (dealii/dealii#308) to output zero-dimensional output features (like particles). It essentially offers the same functionality that Visualization already implements, but for the particles. With this PR the particle/output/ plugins will no longer be necessary once we require deal.II 9.0. @hlokavarapu: This will conflict with #1049. Are you still interested in that functionality? Then we should think about integrating that behavior into this PR.

Things still to do:

  • make sure existing tests are not broken with deal.II 8.5
  • write a changelog entry

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

I like it a lot that this will make the particle output look a lot like the regular field output. Nice work!

What happens to the current code -- does it continue to work if we have an older deal.II? I didn't see that backward compatibility shim, but maybe I missed where you do that.

* of dim (the dimension in which this zero-dimensional particle lives).
*/
template<int dim>
class ParticleOutput : public dealii::DataOutInterface<0,dim>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you have to guard this class with #if DEAL_II_VERSION_GTE(9,0) as well?

{
template<int dim>
ParticleOutput<dim>::~ParticleOutput()
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need this destructor?

dataset_names.push_back(field_name);
else
for (unsigned int component_index=0; component_index<n_components; ++component_index)
dataset_names.push_back(field_name + "[" + Utilities::to_string(component_index) + "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think [ and ] are valid names for data set names. You may have to just append the index with an underscore.

for (unsigned int property_index = 0; property_index < properties.size(); ++property_index)
patches[i].data(property_index+1,0) = properties[property_index];

}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the empty line

@gassmoeller gassmoeller force-pushed the refactor_particle_output branch 2 times, most recently from be910e6 to 2f7639e Compare July 26, 2017 20:48
@gassmoeller
Copy link
Member Author

Yes, I did not check the old functionality. This is fixed now (should work with 8.5.0 and 9.0.pre). I also removed the empty destructor and updated the test results.

@hlokavarapu
Copy link
Contributor

Woah, I have been waiting for this day; compressed particle visualization data, oh man pretty excited :). I don;t want to slow things down but I am still interested in supporting writing out multiple formats for particles. I will export the relevant code of PR #1049 to this code branch. This will take me a day or two.

@gassmoeller
Copy link
Member Author

Sounds good. From a first glance I would think it should fit neatly into the new structure, if you make output_format a list/vector of unique formats, and then loop over the if/else if branches in particles.cc:357-492. I will check in the meantime what happens with the test failures.

@gassmoeller
Copy link
Member Author

This should be good to go. Supersedes #1049.

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Just minor comments.

@@ -70,7 +70,7 @@ end


subsection Postprocess
set List of postprocessors = visualization, velocity statistics, temperature statistics, heat flux statistics, depth average
set List of postprocessors = visualization, velocity statistics, temperature statistics, heat flux statistics, depth average, Stokes residual
Copy link
Contributor

Choose a reason for hiding this comment

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

was this change on purpose?


private:
/**
* Implementation of the base class. See there for information.
Copy link
Contributor

Choose a reason for hiding this comment

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

here and below, I think the proper text ought to be "Implementation of the corresponding interface of the base class."

We do let doxygen import the deal.II documentation. I wonder whether, if you just omit the comment, you get to see the corresponding documentation of the function in the base class. But that's really also not very important to address.

{
const unsigned n_components = property_information.get_components_by_field_index(field_index);
const std::string field_name = property_information.get_field_name_by_index(field_index);
// If it is a 1D element, or a vector, print just the name, otherwise use []
Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise use [] -> otherwise, append the index after an underscore

patches[i].n_subdivisions = 1;
patches[i].data.reinit(property_information.n_components()+1,1);

patches[i].data(0,0) = particle->second.get_id();
Copy link
Contributor

Choose a reason for hiding this comment

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

We must be building the worst-suited data structure to represent particle data here :-) But that's what it is, nothing that can be done about it ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be worthwhile specializing Patch<0,spacedim> to avoid some of the member variables. But that can wait.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gassmoeller
Copy link
Member Author

I addressed the comments. Should be ready to go.

@bangerth bangerth merged commit 98f5b67 into geodynamics:master Aug 8, 2017
@gassmoeller gassmoeller deleted the refactor_particle_output branch August 8, 2017 18:03
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

3 participants