Fixes K->C conversion in `initial_condition`

[navidcy]

This might be related to issues emerging in https://github.com/COSIMA/cosima-recipes/pull/338

But regardless, I believe that numpy.nanmin is more robust and better choice here. What do other reckon?

[angus-g]

Do we have any tests that exercise this K -> C conversion?

[navidcy]

Let's see if the issues discussed in https://github.com/COSIMA/cosima-recipes/pull/338 are healed with this change. If so, we should tag a patch release but also we should add a test that would fail on current main but pass after this PR.

[navidcy]

Do we have any tests that exercise this K -> C conversion?

~I doubt! But if we do, I'll modify them to have "masked" inputs!~ We don't have anything testing the conversion!

[luweiyang]

Here, using np.nanmin did not solve the problem. When I run the notebook using modified regional_mom6.py in my home directory with the following lines:

        if np.nanmin(tracers_out["temp"].isel({"zl": 0})) < 100:
            print(" nanmin temp: ", np.nanmin(tracers_out["temp"].isel({"zl": 0})))
            print("    min temp: ", np.min(tracers_out["temp"].isel({"zl": 0})))
            print(" temp unit conversion from K to C not working!      ")
            # tracers_out["temp"] -= 273.15

I got these lines printed:

nanmin temp: 0.0 min temp: <xarray.DataArray 'temp' ()> array(0.) Coordinates: zl float64 0.5413 temp unit conversion from K to C not working!

So NaNs have been filled with zeros! Any thoughts how?

[navidcy]

interesting @luweiyang! thanks!

could you upload somewhere your notebook so I reproduce your result? I want to load whatever temperature you are loading...

[navidcy]

@luweiyang, why min temp: <xarray.DataArray 'temp' ()> ?

[navidcy]

So I think I found an error in the logic.

On main, here the mask from the initial condition is removed with .bfill and .ffill:

https://github.com/COSIMA/regional-mom6/blob/a0c1fb4c7964b9e1b8ae1b39e25d71e1a4165027/regional_mom6/regional_mom6.py#L726-L736

(There is a comment that promises that is later reinstated... but it's not clear to me where that happens... also after plotting the initial condition I don't see any land mask...)

So there is no NaNs to begin with so the numpy.nanmin wasn't making any sense.

So, one thing at a time. First, the conversion from K -> C is not affected or related with of any interpolation, masks or not, so it should happen first when things are clearer. That's what I did with edc29ee.

Now, @ashjbarnes what happens with that promise on the comment that the mask is reinstated? Is it not? Is it purposely not or is this a glitch? Because now, as you can see in the notebook, after I call expt.initial_condition and then I plot expt.ic_tracers["temp"].isel(zl=0).plot(), the initial condition seems to not have a mask for land!

[navidcy]

@ashjbarnes, any idea about the masking issue I'm asking above?

[ashjbarnes]

Hi Navid, This might be an issue with the regridding, possibly the change to buffer we did a while back! Look at the IC as plotted in your notebook. The Eastward boundary is all zeros. This can happen because there wasn't enough space on this edge to interpolate.

What's confusing me though is that in the notebook it looks like now the temperature is correctly converting to C from K?

[navidcy]

What's confusing me though is that in the notebook it looks like now the temperature is correctly converting to C from K?

Why is it confusing? I fixed the K to C conversion as I explained above. The notebook loads and uses this branch of the package if you notice at the top cells.

[navidcy]

This might be an issue with the regridding, possibly the change to buffer we did a while back! Look at the IC as plotted in your notebook. The Eastward boundary is all zeros.

I don't see that tbh from the notebook. How do you see that they are all zeros? Or did you run the notebook and double checked?

This can happen because there wasn't enough space on this edge to interpolate.

How can we be sure or what's the solution? Could you suggest how to fix it?

[ashjbarnes]

This might be an issue with the regridding, possibly the change to buffer we did a while back! Look at the IC as plotted in your notebook. The Eastward boundary is all zeros.

I don't see that tbh from the notebook. How do you see that they are all zeros? Or did you run the notebook and double checked?

[ashjbarnes]

This can happen because there wasn't enough space on this edge to interpolate.

How can we be sure or what's the solution? Could you suggest how to fix it?

When cutting out the initial condition of access om2 forcing, just add extra room around the edges before passing this on to the package. Then check if there's still a band of zeros

[ashjbarnes]

What's confusing me though is that in the notebook it looks like now the temperature is correctly converting to C from K?

Why is it confusing? I fixed the K to C conversion as I explained above. The notebook loads and uses this branch of the package if you notice at the top cells.

Ok so the only remaining issue is actually about the comment not the code? It can be deleted. There's no need to re-add the NaNs onto the properly re-interpolated boundary condition

[navidcy]

When cutting out the initial condition of access om2 forcing, just add extra room around the edges before passing this on to the package. Then check if there's still a band of zeros

Sorry I'm a bit overwhelmed with many things. Could you help me here? Could you fix the notebook?

Ok so the only remaining issue is actually about the comment not the code? It can be deleted. There's no need to re-add the NaNs onto the properly re-interpolated boundary condition

If the comment is deprecated then we should delete. If the comment was there for a reason but somehow the NaNs were not re-introduced then we should do that. I wasn't sure what was the case... I just noticed the inconsistency between the comment and what followed.

[navidcy]

OK, @ashjbarnes I think we are good here!

Just officially you need to approve since I opened the PR. But unofficially I approve what has happened and what you brought to this PR.