Do not exercise diagnostics when disabled

[charleskawczynski]

This PR allows users to fully disable diagnostics, so that it is not exercised, when disabled. I find myself having to do this locally quite often, so I thought that it might be a good thing to merge.

[szy21]

If the user defines customized diagnostics in the yaml file (e.g. here) but set output_default_diagnostics to false, the model still needs to output diagnostics. We should check that as well?

[charleskawczynski]

If the user defines customized diagnostics in the yaml file (e.g. here) but set output_default_diagnostics to false, the model still needs to output diagnostics. We should check that as well?

There's no documentation/defaults on diagnostics. Perhaps we can/should add a separate flag, enable_diagnostics = true, which can be used to perform this check. I think it would be convenient to have a single switch

[Sbozzolo]

Maybe the more important question is, why do you find yourself disabling this piece of code? What needs to be changed so that you don't have to do that?

[Sbozzolo]

Suggested change

	if config.parsed_args["enable_diagnostics"]
	if !isnothing(scheduled_diagnostics)

[charleskawczynski]

I don't want to exercise this code, that is the point of this PR.

[Sbozzolo]

I don't understand. If you add the suggestion above (in the other code chunk), this code is not exercised when there are no diagnostics

[Sbozzolo]

So, if you want to not exercise this code, you can just comment out the diagnostics from your configuration

[Sbozzolo]

If the user defines customized diagnostics in the yaml file (e.g. here) but set output_default_diagnostics to false, the model still needs to output diagnostics. We should check that as well?

There's no documentation/defaults on diagnostics. Perhaps we can/should add a separate flag, enable_diagnostics = true, which can be used to perform this check. I think it would be convenient to have a single switch

There is documentation. It's here: https://clima.github.io/ClimaAtmos.jl/dev/diagnostics/

And I don't think we should add another redundant switch that effectively just disables other yaml configurations: it makes handling interaction between configurations harder to predict and maintain.

[charleskawczynski]

There is documentation. It's here: https://clima.github.io/ClimaAtmos.jl/dev/diagnostics/

Thanks, sorry, I should have specified that I was referring to the flag in the source code.

Maybe the more important question is, why do you find yourself disabling this piece of code?

Because the diagnostics is still brittle. I already gave feedback on the diagnostics and IntegratorWithDiagnostics design, and you insisted to keep it. All I'm asking for is a way to disable it now, because it's frequently causing problems for me during development. IIRC, the last error I saw was an assumption about the orography type. @bloops recently ran into an issue when we were trying to run ClimaAtmos on the cloud. I'm happy to open issues when they arise, but these snags are slowing development for me and others.

[charleskawczynski]

And I don't think we should add another redundant switch that effectively just disables other yaml configurations: it makes handling interaction between configurations harder to predict and maintain.

It's not redundant in that it ensures that our diagnostics are not exercised, the other flags do not do this. Think of this as a developer flag

[Sbozzolo]

Maybe the more important question is, why do you find yourself disabling this piece of code?

Because the diagnostics is still brittle. I already gave feedback on the diagnostics and IntegratorWithDiagnostics design, and you insisted to keep it. All I'm asking for is a way to disable it now, because it's frequently causing problems for me during development. IIRC, the last error I saw was an assumption about the orography type. @bloops recently ran into an issue when we were trying to run ClimaAtmos on the cloud. I'm happy to open issues when they arise, but these snags are slowing development for me and others.

I am happy to have this PR merged, but please, report these issues and tag me so that we can fix them. I didn't know you or @bloops had problems.

It's not redundant in that it ensures that our diagnostics are not exercised, the other flags do not do this. Think of this as a developer flag

I think that my suggestion is equivalent in terms of code not being exercised but does not add an extra configuration that needs to be maintained.

[charleskawczynski]

I am happy to have this PR merged, but please, report these issues and tag me so that we can fix them. I didn't know you or @bloops had problems.

Thank you, I appreciate your flexibility. I'm happy to open issues. I'll see if I can reproduce the one I ran into this morning.

I think that my suggestion is equivalent in terms of code not being exercised but does not add an extra configuration that needs to be maintained.

Thats fair, but it does require users to specify both flags to disable diagnostics (and the diagnostics flag does not seem to have a default), and those may or may not live in two different files for a particular configuration. I'm happy to (eventually) remove this flag, but I don't see it as adding major overhead, and I think that the name and docs are clear.

[Sbozzolo]

Thats fair, but it does require users to specify both flags to disable diagnostics (and the diagnostics flag does not seem to have a default), and those may or may not live in two different files for a particular configuration. I'm happy to (eventually) remove this flag, but I don't see it as adding major overhead, and I think that the name and docs are clear.

It's not a major overhead, but I would like to try resisting configuration creep, especially when other flags are very close in scope. We have already more than 100 and it is so easy to end up with configurations that are misconfigured and find unexpected results.

The default value for diagnostics is its absence. diagnostics is not a flag, but a nested structure, and it cannot be described in the same way we use for the other flags in default_config.yml. We can probably add it by having it parse as nothing by default, and handle that case specially.