nextsimhub / nextsimdg

neXtSIM_DG : next generation sea-ice model with DG
https://nextsim-dg.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
10 stars 13 forks source link

Move snowfall to IFluxCalculation #154

Closed einola closed 2 years ago

einola commented 2 years ago

For my work on issue #127 having the shared variable snowfall in IFluxCalculation makes more sense than having it in AtmosphereState. I think this is true in general. Can we move snowfall?

timspainNERSC commented 2 years ago

Snowfall makes sense to me as something the atmosphere itself is doing. Instead, might it make sense to have a total precipitation flux or even E-P areal mass flux in the implementations of IFluxCalculation? This seems more 'flux-y' and would parallel the existing subl sublimation mass flux.

timspainNERSC commented 2 years ago

Also, you can just define an array that provides snowfall data within your own module implementation. Since the AtmsophereState code is never called, it will never overwrite snowfall values provided from elsewhere.

    HField snowfall;
…
    registerProtectedArray(ProtectedArray::SNOW, &snowfall);
einola commented 2 years ago

Yes, the problem is that snowfall kind of belongs in both places. You get either

It doesn't make much difference where the snowfall sits, as long as both the flux and atmosphere state codes can modify it.

But can I do that when it's a ProtectedArray? A simple

    registerProtectedArray(ProtectedArray::SNOW, &snowfall);

in the header and

    snowfall[i] = 1.;

doesn't work.

timspainNERSC commented 2 years ago

But can I do that when it's a ProtectedArray? A simple

registerProtectedArray(ProtectedArray::SNOW, &snowfall);

in the header and

snowfall[i] = 1.;

doesn't work.

In what way doesn't work? A ProtectedArray is only protected against anything that accesses it as a ModelArrayRef from writing to the array. In the ModelComponent where it is defined as a ModelArray, there is no concept of protected or shared or write-protected.

What does need to be the case is that the ModelArray is persistent, such as being a class member variable in a class that is still alive. It won't work on variables only defined within a function. (Assuming that's what is going on.)

einola commented 2 years ago

I was trying to modify a protected array - which probably should not be allowed anyway :)

This all becomes a bit annoying and contorted because there are a few layers where information may be coming into the model. What we've done so far is

AtmosphereState <= 2 m temp, etc + precipitation from file or coupler
         ||
         \/
FluxCalculation
         ||
         \/
IceThermodynamics <= Fluxes (total radiative and turbulent and evaporation) + precipitation (rain + snow) from file or coupler

So, whether the fluxes that IceThermodynamics receive comes from AtmosphereState + FluxCalculation or directly from file/coupler depends on the setup. If we consider what's provided we can write it down like this:

Provided by AtmosphereState:

Provided by FluxCalculation or fluxes from file/coupler:

The IceThermodynamics needs everything ice/snow related, while the (bulk) ocean needs the last two ocean-related points. The idea was that you could read from a file or receive from the coupler either fluxes or atmospheric state. If you read/receive atmospheric state, then you can have flexibility in choosing a FluxCalculation implementation.

The problems we have now are:

einola commented 2 years ago

After some thinking and discussion, @timspainNERSC and I have tentatively agreed on the following:

We'll replace AtmosphereState and FluxCalculation modules with an AtmosphereBoundaryConditions module, which will have the following characteristics:

We will then replace the OceanState and bulk ocean module and routines with a similar OceanBoundaryConditions module

brodeau commented 2 years ago

Hi guys, since the flux stuff is my thing I couldn't help and had to catch up on this discussion. I think the proposed merge into a single module is definitively a sound move! /laurent

timspainNERSC commented 2 years ago

It won't really be a single module, but hopefully it will have a more coherent design.

My current design is to place the fields where @einola describes, but to retain the modules AtmosphereState to interface with coupling or climatology and IFluxCalculation to do the atmospheric conditions -> fluxes calculations. At least, retain those modules if they aren't being entirely by-passed.

einola commented 2 years ago

I think that makes sense. That way we can also use a wrapper around Aerobulk for IFluxCalculation - right?

timspainNERSC commented 2 years ago

I think that makes sense. That way we can also use a wrapper around Aerobulk for IFluxCalculation - right?

That was certainly my goal.

timspainNERSC commented 2 years ago

Does these make sense as class hierarchies for the boundary condition classes? And do the names sound reasonable for the different implementations?

einola commented 2 years ago

Yes, the class hierarchies make sense.

For the names, I guess I would rather name them after the source like you did for CoupledOcean. So, then FluxDrivenAtmosphere would be ReanalysisAtmosphere - at least to begin with. We could then see if we want to have one flexible ReanalysisAtmosphere that can eat any reanalysis, or if we want to have ERA5Atmosphere, CFSAtmosphere, etc, etc.

We'll later write FluxCoupledAtmosphere (where the coupler sends fluxes) and StateCoupledAtmosphere (where the coupler sends the atmospheric state), inheriting from IAtmosphereBoundary.

In the meantime, for #127 I will write MonthlyTabulatedAtmosphere (or maybe MU71Atmosphere - referring to the paper), also inheriting from IAtmosphereBoundary.

Does that sound right?

timspainNERSC commented 2 years ago

The current design The interface IAtmosphereBoundary provides the fields

These are the results of the flux calculations that are used by IceGrowth and the current implementations of the lateral spread and vertical thermodynamics.

The interface IOceanBoundary provides the fields