Namelist makeover (Closes #40)

[bss116]

This pull request is a cleanup and restructuring of the input namelists.

• All parameters are described in the file docs/udales-namoptions-overview.md.
• Unused parameters are removed.
• Parameters are categorised, where possible, along the original DALES namelists. Anything related to boundary conditions is moved to the BC section. Any flow forcing is in the physics section. The parameters nblocks and nfcts are now under the section walls.
• Three new sections are introduced: output, trees, chemistry. Output now contains all parameters concerned with outputting instantaneous or averaged fields. Trees and chemistry have all parameters concerning the new tree model, air pollution and air purifiers, but parameters and functionality are currently unchecked.

The test-case nameoptions are updated and should work with these changes, but not the example simulations (separate issue, see #39).

[tomgrylls]

I am happy with the restructuring of namoptions file and with the switches/ parameters that have been removed. There is scope for further cleanup particularly with respect to the functionalities associated with modinlet.f90 and the old statistics routine however these will inevitably come with a more general clean up of unused/ untested legacy parts of the code.

@bss116 does this also align itself with what we want to do in #48? There are many parameters under physics that can be certainly be cleaned up by closing that issue too. Again this can easily be achieved in a later pull request.

If @bss116 is happy that further clean up will be achieved in a more general clean up of the code and via #48 then will approve this.

[bss116]

Great, thanks for reviewing!
Yes exactly, everything related to the flow forcing is currently gathered under PHYSICS and will be further simplified when dealing with issue #48.
The sections TREES, CHEMISTRY, SCALARS and INLET will need to be reviewed after merging in your tree-driver branch.
I also raised separate issues for parameters that we want to remove/check but that require a bit of work -- see #62 and #63.

[bss116]

Further discussion points are listed here: #40 (comment). Should we discuss and address them before merging or raise a separate issue for them?

[tomgrylls]

#40 (comment) raises lots of good points of discussion. I think we should raise a separate issue for these alongside the points I highlighted on legacy parameters associated with modinlet and in relation to the old, redundant statistics routine. Together these encompass quite a lot of good points for a more general clean up of the code. By contruction, by doing this clean up at a later date, namoptions will be further cleaned up. But I am happy with the current developments under this makeover pull request.

I will approve this pull request now. This will also mean that I can proceed with merging my version of the code which will lead to a better state for doing this clean up in the future.

[tomgrylls]

We do have a build error though here it seems?

[dmey]

I am in the middle of something atm -- if it can wait until tonight I am happy to take care of this -- it seems CI related.

[bss116]

@dmey Thanks! Let me know if there is anything to do on my side.
I created a new issue #66 for the further discussion points and the issues @tomgrylls raised in the review. Issue #65 is about the removal of old statistics parameters (they are not in namoptions any more).

[dmey]

The CI should work. The jobs now fail because of https://travis-ci.com/github/uDALES/u-dales/jobs/309337010#L2295.

[bss116]

@dmey Thank you! It makes sense that the comparison fails, because we changed the namelist definitions, therefore the master branch executable will not work with the updated namoptions file. As you can see in the log, the PR branch works fine.

[bss116]

Shall I squash and merge?

[dmey]

@bss116 this is fine by me from the CI side but cannot tell about the namelist changes so it's probably best to ask @tomgrylls

[tomgrylls]

Yes it looks like the simulation runs fine but then it cannot find the fielddump file from the master branch to compare to. I had not realised that the CI uses the master branch executable on the updated branch test files. Would it make more sense to run the master branch executable on its own test files in the future? As long as we do not change the inputs of the test simulation and only its structure like here then this would be preferable?

Happy for this to be squashed and merged.

[bss116]

Agreed, this would make sense, but both options have advantages and disadvantages. We should discuss this when configuring the tests. Could you leave a note on this in #19 or open a new issue?
I'll do the merge now.

[dmey]

@tomgrylls there is actually a bit more to it -- first there is a compile test to check that the version can at least compile, then there is a regression test (perhaps the more important one) to check that the new changes do not change the results. To do this we need to have a reference and this is where the one in master comes into play. You can change this of course but since you are merging into master it would make sense to compare against master. In this case the job fails as the results do differ -- it is then up to the user to decide if these changes are intended or not. We can/should probably add some docs and discuss it a bit more but let's do this in a different issue.

[tomgrylls]

Agreed. I will just leave a comment on #19.