[Bugfix] add an option that fixes temperature/composition only on inflow/close… #2419

jdannberg · 2018-06-24T18:44:38Z

…d boundaries, but not on outflow boundaries.

Fixes #903.

We can discuss what the default for the temperature equation should be.

And I still have to add a changelog entry (which I will do after we've agreed on a the default for the temperature).

tjhei · 2018-06-25T04:43:59Z

source/simulator/core.cc

+    // Now check if the current_constraints we just computed changed from before. Do this before the melt handler
+    // adds constraints for the melt cells, because they are allowed to change (and often do) without us having
+    // to reinit the sparsity pattern.
+    bool constraints_changed = false;


move this inside the if (!mesh_has_changed)

tjhei · 2018-06-25T04:44:57Z

source/simulator/core.cc

+    bool mesh_has_changed = false;
+    if (!(current_constraints.get_local_lines().size() == new_current_constraints.get_local_lines().size()))
+      mesh_has_changed = true;
+    else if (!(current_constraints.get_local_lines() == new_current_constraints.get_local_lines()))


if ((A.size() != B.size()) || (A != B)) mesh_has_changed =true;

tjhei · 2018-06-25T05:00:12Z

source/simulator/core.cc

+    current_constraints.clear ();
+    current_constraints.reinit (introspection.index_sets.system_relevant_set);
+    current_constraints.merge (new_current_constraints);
+#endif


add current_constraints.close(); before endif

tjhei · 2018-06-25T05:02:02Z

source/simulator/core.cc

+    current_constraints.clear ();
+    current_constraints.reinit (introspection.index_sets.system_relevant_set);
+    current_constraints.merge (new_current_constraints);
+#endif

    if (time_step == 0 && parameters.include_melt_transport)
      melt_handler->add_current_constraints (current_constraints);


I am worried that the melt handler will break our logic: if we add constraints here, we will rebuild the sparsity pattern even though that shouldn't be necessary. I am not quite sure how to fix this. Maybe ignore p_c indices in the check above?!

jdannberg · 2018-06-25T18:49:10Z

There were some tests failing, and I mostly modified the input files so that they now exactly reproduce the old behaviour. The only change in the tests I updated was the memory consumption of the constraints, so I guess this is expected.

I also addressed the comments. Can someone have another look?

bangerth

I read through the first part, but it's late. I'll check the rest tomorrow, after you've already addressed some of the initial comments.

bangerth · 2018-06-26T05:44:18Z

include/aspect/boundary_composition/interface.h

+         * on parts of the boundaries where material flows out.
+         */
+        bool
+        allows_fixed_composition_on_outflow_boundaries() const;


All other names here are in the infinitive form, like in find_boundary_... or get_fixed_.... Maybe change to allow_fixed_...?

I modeled this after other functions we have that return bools, like has_boundary_temperature, simulator_is_initialized, pressure_rhs_needs_compatibility_modification etc. If the function returns a bool, I think this naming scheme makes sense, as you would use it in statements like
if (boundary_composition_manager.allows_fixed_composition_on_outflow_boundaries()).
That seems more intuitive than
if (boundary_composition_manager.allow_fixed_composition_on_outflow_boundaries()).

I see -- it's a predicate (you're asking a true/false question) rather than a request for information. OK then.

bangerth · 2018-06-26T05:45:02Z

include/aspect/boundary_composition/interface.h

+         * Whether we allow the composition to be fixed on parts of the boundary
+         * where material flows out of the domain.
+         */
+        bool allow_fixed_composition_on_outflow_boundaries;


You'll have to change this name then since it'll clash with the changed function name.

bangerth · 2018-06-26T05:55:34Z

include/aspect/boundary_temperature/interface.h

+         * on parts of the boundaries where material flows out.
+         */
+        bool
+        allows_fixed_temperature_on_outflow_boundaries() const;


same here, obviously

bangerth · 2018-06-26T06:07:36Z

include/aspect/simulator.h

+      /**
+       * Offset the boundary id of all faces located on an outflow bounday
+       * by a fixed value given by the input parameter
+       * @param boundary_id_offset.


I dislike that this is a parameter. That's something that will be completely bizarre to basically every user who does not look up the code.

I'd much rather than you simply disallow a certain range of boundary indicators (and add a check somewhere) and then do the shift internally.

This is how I had it in the beginning, but I need the offset in two functions (replace_outflow_boundary_ids and reset_outflow_boundary_ids), and it has to be the same in both. This way I at least make sure that the same number is used in both places. Do you have a good suggestion on how to do this?

I think you already hard-coded it. That's ok. I know how to fix it later on.

bangerth · 2018-06-26T06:07:51Z

include/aspect/simulator.h

@@ -1193,6 +1193,25 @@ namespace aspect
                                        LinearAlgebra::BlockVector &relevant_dst,
                                        LinearAlgebra::BlockVector &tmp_distributed_stokes);

+      /**
+       * Offset the boundary id of all faces located on an outflow bounday


-> boundary

bangerth · 2018-06-26T06:08:35Z

include/aspect/simulator.h

+       * This function is implemented in
+       * <code>source/simulator/helper_functions.cc</code>.
+       */
+      void reset_outflow_boundary_ids(const unsigned int boundary_id_offset);


I think "restore" would be a better word.

bangerth · 2018-06-26T06:09:43Z

source/boundary_composition/interface.cc

+                           "When the composition is fixed on a given boundary as determined "
+                           "by the list of 'Fixed composition boundary indicators', there "
+                           "might be parts of the boundary where material flows out and "
+                           "one may want to prescribe the composition only on the parts of "


the -> those

bangerth · 2018-06-26T06:09:54Z

source/boundary_composition/interface.cc

+                           "by the list of 'Fixed composition boundary indicators', there "
+                           "might be parts of the boundary where material flows out and "
+                           "one may want to prescribe the composition only on the parts of "
+                           "the boundary where there is inflow. This parameter determines "


the -> those

I don't understand this comment (but fixed the one above for line 334). Was this what you meant?

It's just a question of emphasis. "only on the parts the boundary" and "only on those parts of the boundary" means the same, it just provides additional emphasis.

bangerth · 2018-06-26T06:16:18Z

source/boundary_composition/interface.cc

+                           "the boundary where there is inflow. This parameter determines "
+                           "if compositions are only prescribed at these inflow parts of the "
+                           "boundary (if false) or everywhere on a given boundary, independent "
+                           "of the flow direction (if true).");


Maybe add the following paragraph to this:

"Mathematically speaking, the compositional fields satisfy an advection equation that has no diffusion. For this equation, one can only impose Dirichlet boundary conditions (i.e., prescribe a fixed compositional field value at the boundary) at those boundaries where material flows in. This would correspond to the true'' setting of this parameter, which is correspondingly the default. On the other hand, on a finite dimensional discretization such as the one one obtains from the finite element method, it is possible to also prescribe values on outflow boundaries, even though this may make no physical sense. This would then correspond to the false'' setting of this parameter."

bangerth · 2018-06-26T06:17:45Z

source/boundary_temperature/interface.cc

+                           "the boundary where there is inflow. This parameter determines "
+                           "if temperatures are only prescribed at these inflow parts of the "
+                           "boundary (if false) or everywhere on a given boundary, independent "
+                           "of the flow direction (if true).");


here add something similar, except that it is necessary to state that mathematically it is allowed to prescribe the temperature even on outflow boundaries as long as the diffusion coefficient is nonzero. In practice, however, this would only make physical sense if the diffusion coefficient is actually quite large to prevent the creation of a boundary layer.

jdannberg · 2018-06-26T17:07:40Z

@bangerth, I think I addressed all of your comments, so it would be great if you could have another look.

bangerth

One comment before I get to the details: I think "Allow fixed composition/temperature" is maybe not the right term for the parameter/member variable/member function because the field does not have to be fixed -- which to me would imply that it is constant in time. How about you replace "fixed" by "prescribed"?

bangerth · 2018-06-27T17:46:50Z

include/aspect/boundary_composition/interface.h

+         * on parts of the boundaries where material flows out.
+         */
+        bool
+        allows_fixed_composition_on_outflow_boundaries() const;


I see -- it's a predicate (you're asking a true/false question) rather than a request for information. OK then.

jdannberg · 2018-06-27T18:06:45Z

For the name of the parameter, I just wanted to make that consistent with the rest of the parameters/code. For the temperature and composition, we call the input parameter Fixed temperature boundary indicators and Fixed composition boundary indicators (and the same is true for the names of the variables in the code). You can argue about how much sense those names make, but my new parameter refers to these old names. So I think "Allow fixed composition/temperature on outflow boundaries" is the right choice here.

bangerth · 2018-06-27T18:14:05Z

Ah, so much historical baggage :-( In that case, leave it as you have it but maybe make a note in the doc of the parameter that fixed = Dirichlet, not time independent.

bangerth

Here my remaining comments.

Let's postpone the discussion about the replace/restore functions to a follow-up. I can do this myself; I think that will be faster than trying to explain how I would like that function to look like.

bangerth · 2018-06-27T18:19:23Z

source/simulator/core.cc

+    // we update the boundary indicators of all faces that belong to ouflow boundaries
+    // so that they are not in the list of fixed composition boundary indicators any more.
+    // We will undo this change in a later step, after the constraints have been set.
+    if (!boundary_composition_manager.allows_fixed_composition_on_outflow_boundaries())


this is a copy of the comment from above except that you want to apply it for the compositional fields now. maybe say something along the lines "Use the same trick for marking up outflow boundary conditions for compositional fields as we did above already for the temperature."

I guess you convinced yourself that you need the correct boundary indicators in the call to update above in line 751 (new) and that you can't just avoid the restore/replace pair in line 747/758?

No, I just thought it would be cleaner like this. There are different parameters in the if-statement, so we would have to do something like
if (!boundary_temperature_manager.allows_fixed_temperature_on_outflow_boundaries() && boundary_composition_manager.allows_fixed_composition_on_outflow_boundaries()) restore_outflow_boundary_ids(boundary_id_offset); if (!boundary_composition_manager.allows_fixed_composition_on_outflow_boundaries() && boundary_temperature_manager.allows_fixed_temperature_on_outflow_boundaries()) replace_outflow_boundary_ids(boundary_id_offset);

Ah, I missed that. I thought you're just restoring it for the update call immediately above.

OK then, just edit the comment.

bangerth · 2018-06-27T18:22:53Z

source/simulator/core.cc


    // let the melt handler add its constraints once before we solve the porosity system for the first time
    if (parameters.include_melt_transport)
-      melt_handler->save_constraints (current_constraints);
+      melt_handler->save_constraints (new_current_constraints);


this is pre-existing, but it still makes me unsure: the function is called "save_" which suggests that the melt handler stores a copy somewhere. Is this correct? Does it copy, or does it keep a reference? Keeping a reference will not work any more, of course.

I wonder about this because the comment in front of the line does not seem to be in sync with the code -- it talks about adding constraints, when the function is called "save_". There is also a comment a couple of lines below in which you say "Do this before the melt handler adds constraints...", which seems to be in conflict with the comment here.

The melt handler copies the current constraints and stores them in a private member variable so that we can use them later. This is necessary because we want to add the melt constraints, which depend on the solution of the porosity field, later on, after we have computed this solution. In this way, we only need to update the constraints matrix instead of computing all constraints again.
(at least this is what the documentation in melt.h says)

But you are right, the comment refers to a different line, and I'll update it.

bangerth · 2018-06-27T18:34:31Z

source/simulator/core.cc

+    // If the mesh got refined and the size of the linear system changed, the old and new constraint
+    // matrices will have different entries, and we can not easily compare them.
+    // However, in case of mesh refinement we have to rebuild the matrix anyway, so we can skip the
+    // step that checks if constraints have changed.


couldn't you do this easier by invalidating the current_constraints object at the time of mesh refinement?

@tjhei designed this part and can answer that question much better than I can.

Let's not worry about it then.

bangerth · 2018-06-27T18:38:44Z

source/simulator/core.cc

+    // step that checks if constraints have changed.
+    bool mesh_has_changed = false;
+    if (!(current_constraints.get_local_lines().size() == new_current_constraints.get_local_lines().size())
+        || !(current_constraints.get_local_lines() == new_current_constraints.get_local_lines()))


Do you really need both comparisons? I would have expected that if you ask for the two objects to be the same (second line), that the sizes must also be the same (first line).

Yes, but if the sizes are different (first line) the code will crash if I just call the second line.

You're right.

I think you could still make this easier to read by using a != b instead of !(a==b).

bangerth · 2018-06-27T18:39:17Z

source/simulator/core.cc

+    bool mesh_has_changed = false;
+    if (!(current_constraints.get_local_lines().size() == new_current_constraints.get_local_lines().size())
+        || !(current_constraints.get_local_lines() == new_current_constraints.get_local_lines()))
+      mesh_has_changed = true;


You can roll the condition you use in the if here right into the initialization of the mesh_has_changed variable.

bangerth · 2018-06-27T18:41:26Z

source/simulator/core.cc

+#if DEAL_II_VERSION_GTE(9,0,0)
+        for (auto &row: current_constraints.get_lines())
+          {
+            if (!new_current_constraints.is_constrained(row.index))


All you check here is whether the set of constrained DoFs is the same, but not whether the constraint weights are the same. Is this on purpose? If so, maybe not call the variable constraints_changed but constrained_dofs_set_changed?

Yes. Can you leave a note, @jdannberg ? This is to decide if we need to construct a new sparsity pattern. The matrices will be reassembled in each timestep regardless and the value of the constraints does not matter for the sparsity pattern.

bangerth · 2018-06-27T18:42:34Z

source/simulator/core.cc

+        for (const auto row: current_constraints.get_local_lines())
+          {
+            if (current_constraints.is_constrained(row)
+                != new_current_constraints.is_constrained(row))


With these conditions here (for 8.5, and the corresponding one for 9.0), you really only change that the set of constrained DoFs we have currently is a subset of the ones we had before. But there may have been more before. Is that what you want to test? If you really want equality, then you also need to check the other way around.

I think we concluded we don't have to check the other way around because if you only add constraints you don't have to rebuild the matrix. But I may have gotten that wrong, @tjhei, can you add to that?

That seems wrong. New constraints may lead to new entries in the matrix.

I agree that both versions should do the same thing (even though either way will work)

bangerth · 2018-06-27T18:54:10Z

source/boundary_composition/interface.cc

+                           "if compositions are only prescribed at these inflow parts of the "
+                           "boundary (if false) or everywhere on a given boundary, independent "
+                           "of the flow direction (if true)."
+                           "Mathematically speaking, the compositional fields satisfy an "


add "\n\n" to provide offset here.

maybe do the same for the corresponding temperature entry.

jdannberg · 2018-06-28T00:36:10Z

Okay, I think I addressed all of @bangerth's comments, except for the three marked for @tjhei. So @tjhei, maybe you can have a look?

jdannberg · 2018-06-29T00:57:19Z

Now that I think about it, for the porosity and any other fields that are advected with the melt velocity, we should probably use the melt velocity to decide if the boundary is an outflow boundary, correct?

gassmoeller · 2018-06-29T21:26:40Z

Yes, whatever velocity is used in the advection term.

jdannberg · 2018-07-01T17:35:23Z

Okay, I thought about this, and there's another problem with that. In most models with melt transport, there is the porosity (which is transported with the melt velocity) and other compositional fields (like the depletion) that are transported with the solid velocity. At the moment, we can not prescribe different boundary conditions for different compositional fields. So we can only handle the outflow boundaries correctly for either the melt or the solid fields.

I don't really know what to do about this, except to enforce that 'Allow fixed composition on outflow boundaries' is set to true in models with melt and fix this later.

tjhei · 2018-07-05T22:58:36Z

At the moment, we can not prescribe different boundary conditions for different compositional fields.

Yes, the problem is that we reuse the same sparsity pattern to avoid having to store n matrices for n compositional fields. One option would be to create the sparsity pattern only with the constraints that are relevant for all compositional fields and add the other constraints later on. This would waste a tiny amount of memory because most of the entries of those rows are zero, but I think we could care less in that situation...

…d boundaries, but not on outflow boundaries

- simplify logic of recomputing melt sparsity pattern - this should speed up melt computations because rebuild is only done if melt cells change

jdannberg · 2018-07-22T20:33:05Z

Okay, for now I disabled this in models with melt by default (because I don't have the time to implement what @tjhei suggested in the coming weeks, and fixing this in all other models is better than not fixing it at all).
It's not very pretty, so @gassmoeller let me know if there's a better solution.

gassmoeller · 2018-10-02T20:11:41Z

@tjhei: for this PR your tester gives different results than the CIG tester, because master changed since this branch was created. We can only use the CIG tester results to update, because yours would not pass on master. Do you prefer it this way? We were confused by the differences for a few minutes. Do you want to change your tester configuration to test the merged version of the PR? The setting is under Branch Sources -> Github -> Discover pull requests from forks -> Strategy -> Merging the pull request with the current target branch revision.

tjhei · 2018-10-03T14:20:32Z

This could be fixed be rebasing to the current master, right? This can be confusing, I admit. I set it up this way on purpose because it mirrors exactly what the old tester did before. I think there are pros and cons to both approaches, but let me think about it.

tjhei · 2018-10-03T14:22:44Z

If we test pr-head then we might break master when merging a passing PR. But if we test pr-merge, things behave differently on your local machine and the tester, which can be confusing too. For example, the tester might not even compile while things work locally.

gassmoeller

Just a few minor comments left. Ready to merge after you addressed them.

gassmoeller · 2018-10-04T17:10:10Z

source/simulator/helper_functions.cc

+  void
+  Simulator<dim>::replace_outflow_boundary_ids(const unsigned int offset)
+  {
+    const QGauss<dim-1> quadrature_formula (finite_element.base_element(introspection.base_elements.temperature).degree+1);


this should be the velocity element, as you evaluate velocity below

Not sure. This is used to determine if the face has outflow when assembled for the temperature (or compositional) equation.

Ah, disregard my comment then. There are identical by default anyway.

gassmoeller · 2018-10-04T17:30:37Z

source/simulator/core.cc

+        for (const auto row: current_constraints.get_local_lines())
+          {
+            if (current_constraints.is_constrained(row)
+                != new_current_constraints.is_constrained(row))


I think Juliane and Timo are right here. Apart from the case where no constraints change, we have to consider two cases:

The dof was constrained before and is not longer constrained in new_current_constraints. That means its relevant matrix entries will now be assembled and we have to ensure they are present. This is enforced by the current implementation.

The dof was not constrained before, and is now constrained in new_current_constraints. In this case the matrix entries exist, but they are not longer touched (as they exist in the constraint matrix), and therefore we can as well skip recreating the matrix (which is what is currently happening).

We did a test that worked although it should have triggered this bug (no_dirichlet_on_outflow_only_new_constraints) and so I am relatively confident the current implementation is correct.

gassmoeller · 2018-10-04T17:46:29Z

source/simulator/core.cc

    current_constraints.close();
+#endif

    // let the melt handler add its constraints once before we solve the porosity system for the first time


You moved or removed the following lines (because you do not need them any more?), please also remove the comment.

gassmoeller · 2018-10-04T17:47:07Z

source/simulator/helper_functions.cc

+    FEFaceValues<dim> fe_face_values (*mapping,
+                                      finite_element,
+                                      quadrature_formula,
+                                      update_gradients      | update_values |


you do not use the gradients, please remove the update flag

gassmoeller · 2018-10-04T17:48:36Z

source/simulator/helper_functions.cc

+            if (face->at_boundary())
+              {
+                Assert(face->boundary_id() <= offset,
+                       ExcMessage("You can not use more than 10000 boundary indicators!"));


I think 10000 is not the correct number, somewhere above you write 128?

Yes, the error should be something like "To allow 'Allow fixed composition on outflow boundaries' you are only allowed to use boundary ids between 0 and ".

Actually, the error should be "If you do not allow fixed composition on outflow boundaries you are only allowed...' I'll update it.

gassmoeller · 2018-10-04T17:49:51Z

source/simulator/solver_schemes.cc

-
-        setup_system_matrix (introspection.index_sets.system_partitioning);
-        setup_system_preconditioner (introspection.index_sets.system_partitioning);
+        this->compute_current_constraints();


you do not need this-> in this line.

gassmoeller · 2018-10-04T17:52:37Z

tests/no_dirichlet_on_outflow_C.prm

@@ -0,0 +1,178 @@
+# This test checks if fixed composition boundary conditions are only 
+# prescribed on outflow boundaries (if this option is activated in 
+# the input file), and in particular if this also works foe time-


gassmoeller · 2018-10-04T17:56:26Z

@tjhei: You are right there are good reasons for both. I guess in the end we should just do both, and keep the difference in mind. When we find a way to circumvent the DNS issues with the CIG tester we can activate both test setups, which should make the differences more obvious (if they give different results).

gassmoeller · 2018-10-05T21:19:12Z

Ok, looks like all comments have been addressed. Ready to merge.

gassmoeller · 2018-10-05T21:20:52Z

@tjhei: Are you fine with the changes as well?

…tflow [Bugfix] add an option that fixes temperature/composition only on inflow/close…

tjhei reviewed Jun 25, 2018

View reviewed changes

jdannberg force-pushed the no_dirichlet_on_outflow branch from cdfcdee to 7a51cfc Compare June 25, 2018 18:06

bangerth reviewed Jun 26, 2018

View reviewed changes

jdannberg force-pushed the no_dirichlet_on_outflow branch from 9e6da5a to af84bdf Compare June 26, 2018 22:21

bangerth reviewed Jun 27, 2018

View reviewed changes

jdannberg and others added 9 commits July 22, 2018 20:52

add an option that fixes temperature/composition only on inflow/close…

3d91b10

…d boundaries, but not on outflow boundaries

make 8.5 compatible

22ff9bb

address comments and fix tests

68e8dc8

address comments

d495ae2

address comments

fca78f1

melt handling of constraints

98ca1e1

- simplify logic of recomputing melt sparsity pattern - this should speed up melt computations because rebuild is only done if melt cells change

fixup

e461a9b

add test for melt model

22657d0

disable composition parameter for models with melt

042621b

jdannberg force-pushed the no_dirichlet_on_outflow branch from 9813b00 to 042621b Compare July 22, 2018 20:29

jdannberg added 3 commits August 11, 2018 20:45

fix parameter name

8e61f04

add a new test

22becbf

fix tests to take into account changes on master

16c0ce8

jdannberg force-pushed the no_dirichlet_on_outflow branch from e2fd5a7 to 16c0ce8 Compare October 2, 2018 20:09

gassmoeller approved these changes Oct 4, 2018

View reviewed changes

address comments

1a1f5b8

tjhei approved these changes Oct 6, 2018

View reviewed changes

tjhei merged commit b729283 into geodynamics:master Oct 6, 2018

jdannberg mentioned this pull request Oct 24, 2018

[bugfix] fix restarting melt models #2691

Merged

freddrichards pushed a commit to freddrichards/aspect that referenced this pull request May 20, 2019

Merge pull request geodynamics#2419 from jdannberg/no_dirichlet_on_ou…

0eabeaa

…tflow [Bugfix] add an option that fixes temperature/composition only on inflow/close…

[Bugfix] add an option that fixes temperature/composition only on inflow/close… #2419

[Bugfix] add an option that fixes temperature/composition only on inflow/close… #2419

Conversation

jdannberg commented Jun 24, 2018

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

jdannberg commented Jun 25, 2018

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

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

jdannberg commented Jun 26, 2018

bangerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdannberg commented Jun 27, 2018

bangerth commented Jun 27, 2018

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

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

jdannberg commented Jun 28, 2018

jdannberg commented Jun 29, 2018

gassmoeller commented Jun 29, 2018

jdannberg commented Jul 1, 2018

tjhei commented Jul 5, 2018

jdannberg commented Jul 22, 2018

gassmoeller commented Oct 2, 2018

tjhei commented Oct 3, 2018

tjhei commented Oct 3, 2018

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

gassmoeller commented Oct 4, 2018

gassmoeller commented Oct 5, 2018

gassmoeller commented Oct 5, 2018