Code review comment for lp:~mapdes/fluidity/firedrake-extrusion

On the Fortran side:

Why do you add a check for lshape%type /= -666 in make_mesh?

In make_global_numbering_new:

extrusion is a logical and should be declared as such:

logical :: extrusion

extrusion = .true. etc...

Why introduce dofs_per_stack? What was wrong with dofs_per. They're the same shape are they not? If not, I don't see how things work.

In topology_dofs: Remove the debugging (commented out) dof_old and halo_dof_old stuff, unless it's supposed to actually do something.

Is it clear why we don't use mesh%nodes = total_dofs in both extruded and unextruded cases? It seems to me like they should be the same.

In the python code:

The magic here is horribly underdocumented. A description of the shape of the key datastructures in comments will be very useful if anyone else is to ever have a chance of fixing any bugs.

is it clear why you insist on building fixed-length lists which you then immediately convert into numpy arrays? numpy.zeros(size, dtype=foo) is much better than numpy.array([0]*size, dtype=foo).

Since the ExtrudedMesh class inherits from Mesh it shouldn't have to reimplement all the superclass methods except in the cases where they are different. Why does it? It's imperative to document what the extrusion kernel does, and so on.

Why does fiat_element_from_ufl_element no longer take a ufl_cell as an argument?

Just because FunctionSpace was underdocumented before, adding new functionality is a perfect time to add a docstring. Also, I think it should be an error to pass in vfamily as well as family if the mesh is not an extruded mesh, just for error checking.

Why is the degree now optional? I thought that family was just a string so it can't contain any extra information. But it seems like the extruded case the family is not a string. This /definitely/ needs documenting. I guess the family is now a cross of function spaces? In any case, there's now no error checking if the degree is not passed in an you've asked for a normal function space. So you need to fix that.

op2.Set instantiation does the right thing if layers is passed as None (it sets layers to 1 internally). So there's no need to do that again before Set instantiation.

Python has a != operator, so can you write foo != bar rather than not (foo == bar)?

Function output in the extruded case looks like it's going to be hideously bad, I note that you comment this. Maybe splat a big warning to the screen for now too?

Test directories should be named firedrake_foo with the test python file named test_firedrake_foo.py (so that we can import more than one test in the same python interpreter). As dham says, don't put mesh files in, use the builtin mesh generation routines.

In the tests, why do you return values as strings? Is it for floating point comparison purposes? If so, don't do that, use numpy.allclose. If not, why?

Why are the tolerances on some existing tests altered?

The helmholtz test comments don't match the code. Also, it doesn't actually /test/ anything. Is it a demo?