New ocean conservation task

[cbegeman]

This PR adds the capability to create time series plots representing various terms from the ocean's conservationCheck analysis member. This capability is given as a new task with its own config section conservation, but appears as a section in the Time series gallery when analysis is generated.

Generally, the variables from conservationCheckOutput stream are unmodified when they are plotted. The modifications that this code makes to some variables are:

• changing units
• time-averaging (using the existing capability in the time series task)
• generating a cumulative sum of fluxes, e.g. the mass change from land ice components

Example output is located here:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.cbegeman/analysis/20240223.v3.LR.CRYO1850-DISMF.paolo.chrysalis/ocean/index.html#timeseries

[cbegeman]

@xylar This is about ready to review but I'm having trouble with endYear = end that I haven't nailed down. Do you have any insight? This is the only change I could think to make: 01c8887

[xylar]

@cbegeman, yes, that's the way to handle end. During the setup phase of MPAS-Analysis, we look through the files that are actually available and replace the config options that were originally end with the last full year available in the input data. Since you have added a new config section that has an end year and which defaults to end, that section needs to be added to the function that makes the end substitution as you have done.

[cbegeman]

@xylar Thanks. Then it's ready to review. Let me know if you see any errors associated with end. Maybe it was just a fluke on my end.

[cbegeman]

FYI, I'll probably change the default behavior in default.cfg and the behavior in polar_regions w.r.t. which conservation plots are generated. Happy to take suggestions there.

[xylar]

Let me know if you see any errors associated with end. Maybe it was just a fluke on my end.

Could you post the error you are getting?

[cbegeman]

I don't have it handy but it was something about endYear not being an integer, presumably because it was still end. It could be because I didn't purge before testing 01c8887.

[darincomeau]

Looks like the black line run doesn't start until 6 months in?

https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.cbegeman/analysis/20240223.v3.LR.CRYO1850-DISMF.paolo.chrysalis/ocean/index.html#timeseries&gid=1&pid=9

Are the units here m instead of mm, there's the 1e-5 scale?

[cbegeman]

Are the units here m instead of mm, there's the 1e-5 scale?

I think the conversion here is right.

MPAS-Analysis/mpas_analysis/ocean/conservation.py

Lines 519 to 523 in efacd8f

	# Convert from to Gt to kg
	land_ice_mass_change = land_ice_mass_change * 1e12
	rho = self.namelist.getfloat('config_density0')
	# Convert from to kg to mm
	derived_variable = land_ice_mass_change * 1.0e3 / (rho * A)

If you look at the mass fluxes, they are also small:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.cbegeman/analysis/20240223.v3.LR.CRYO1850-DISMF.paolo.chrysalis/ocean/index.html#timeseries&gid=1&pid=8

[darincomeau]

ok, I was expecting a trend in Paolo DISMF of around -0.2 mm/yr, but I guess with these first few years it's super early. Thanks for checking!

[cbegeman]

@xylar Here we go. Still a problem after purging:

Traceback (most recent call last):
  File "/lcrc/group/e3sm/ac.cbegeman/scratch/MPAS-Analysis-worktrees/enhance-ssh-analysis/mpas_analysis/__main__.py", line 448, in add_task_and_subtasks
    analysisTask.setup_and_check()
  File "/lcrc/group/e3sm/ac.cbegeman/scratch/MPAS-Analysis-worktrees/enhance-ssh-analysis/mpas_analysis/ocean/conservation.py", line 159, in setup_and_check
    self.endYear = self.config.getint('conservation', 'endYear')
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.cbegeman/miniconda3/envs/mpas_dev_ssh/lib/python3.12/site-packages/mpas_tools/config.py", line 136, in getint
    return self.combined.getint(section, option)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.cbegeman/miniconda3/envs/mpas_dev_ssh/lib/python3.12/configparser.py", line 796, in getint
    return self._get_conv(section, option, int, raw=raw, vars=vars,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.cbegeman/miniconda3/envs/mpas_dev_ssh/lib/python3.12/configparser.py", line 786, in _get_conv
    return self._get(section, conv, option, raw=raw, vars=vars,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.cbegeman/miniconda3/envs/mpas_dev_ssh/lib/python3.12/configparser.py", line 781, in _get
    return conv(self.get(section, option, **kwargs))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: 'end'
Warning: oceanConservation failed during check and will not be run

1 tasks and subtasks failed during setup.

Running tasks: 100% |#                                                                                                          | ETA:  --:--:--

Log files for executed tasks can be found in /lcrc/group/e3sm/ac.cbegeman/MPAS-Analysis-results/20240201.v3.SORRM.piControl.chrysalis//logs
Total setup time: 0:00:05.86
Total run time: 0:00:05.87
Generating webpage for viewing results...
Web page: https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.cbegeman/analysis/20240201.v3.SORRM.piControl.chrysalis/

[cbegeman]

Looks like the black line run doesn't start until 6 months in

I think that's because of the 12-month averaging window but now that you mention it, there is a different starting point for the two curves. I'll look into it.

[xylar]

@cbegeman, I think the issue with end will be fixed if you add conservation to:
https://github.com/cbegeman/MPAS-Analysis/blob/new-ocn-conservation-task/mpas_analysis/shared/analysis_task.py#L223

However, at this point I'm wondering if we really want different time bounds on the conservation time series than other time series. For example, when folks run through zppy, there will only be one set of time bounds for time series regardless of whether that's "normal" time series, the El Nino index, or conservation. Maybe it's more trouble than it's worth to have separate time bounds.

[xylar]

Looks like the black line run doesn't start until 6 months in?

This is presumably because it's a centered annual rolling mean so it starts half a year in.

[cbegeman]

Looks like the black line run doesn't start until 6 months in

I think that's because of the 12-month averaging window but now that you mention it, there is a different starting point for the two curves. I'll look into it.

It looks like something funky is happening when time averaging is enabled. The run without time averaging looks fine. I think this issue is outside the scope of this PR. @xylar would you agree?

[cbegeman]

@cbegeman, I think the issue with end will be fixed if you add conservation to:
https://github.com/cbegeman/MPAS-Analysis/blob/new-ocn-conservation-task/mpas_analysis/shared/analysis_task.py#L223

However, at this point I'm wondering if we really want different time bounds on the conservation time series than other time series. For example, when folks run through zppy, there will only be one set of time bounds for time series regardless of whether that's "normal" time series, the El Nino index, or conservation. Maybe it's more trouble than it's worth to have separate time bounds.

I went back and forth about this. Is it too confusing though if we have a separate [conservation] section but all of the config options except plotTypes are derived from [timeSeries]? I originally thought about just adding the conservation time series to the timeSeries task but that seemed impractical because the conservation variables are derived from different streams/output files.

[xylar]

It looks like something funky is happening when time averaging is enabled. The run without time averaging looks fine. I think this issue is outside the scope of this PR. @xylar would you agree?

@cbegeman, I don't see an obvious issue. There's a really strong seasonal cycle and it's nearly impossible to know what the running mean should look like once it's removed, so to me the 2 plots could be totally consistent with one another.

[xylar]

I went back and forth about this. Is it too confusing though if we have a separate [conservation] section but all of the config options except plotTypes are derived from [timeSeries]? I originally thought about just adding the conservation time series to the timeSeries task but that seemed impractical because the conservation variables are derived from different streams/output files.

I think it's totally fine if most of the config options are from [timeSeries] and only a few are from [conservation]. It's understood that many analysis tasks work that way. For example, every climatology map doesn't get to set its own time bounds on the climatology.

[cbegeman]

@cbegeman, I don't see an obvious issue. There's a really strong seasonal cycle and it's nearly impossible to know what the running mean should look like once it's removed, so to me the 2 plots could be totally consistent with one another.

@xylar The issue is that the black line above doesn't extend all the way to 01-06.

[cbegeman]

I think it's totally fine if most of the config options are from [timeSeries] and only a few are from [conservation]. It's understood that many analysis tasks work that way. For example, every climatology map doesn't get to set its own time bounds on the climatology.

Great. I'll make this change then. I will also change the name of the config section to timeSeriesConservation to make this overlap clear.

[xylar]

@xylar The issue is that the black line above doesn't extend all the way to 01-06.

Ah, I hadn't noticed that. I agree, that's weird. We should look into that.

[xylar]

@cbegeman, after fiddling around with ways to solve the end problem, I decided to just drop support for that in #991. Hopefully, you can rebase onto develop once that goes in and there will be less pain and suffering for future developers.

[cbegeman]

@darincomeau Thanks for pointing out the small values. There was an issue with the units conversion with clauses such as if 'mass_flux' in plot_type. I fixed the bug and decided to reorganize the code a bit cd49f61.

[cbegeman]

@xylar The issue is that the black line above doesn't extend all the way to 01-06.

I'm not sure what change solved the problem, but it appears to be solved now.

[xylar]

I'm not sure what change solved the problem, but it appears to be solved now.

Great! I was hoping so!

[darincomeau]

I don't have comments on the implementation, but the end result looks great!
Thanks @cbegeman , this is going to be so nice to have!

[xylar]

One small fix. Otherwise, things look good as far as I can tell on my phone. Please merge when this is fixed or ping me and I'll do it.

[cbegeman]

@xylar This is ready to merge when you have a chance. I have retested with the latest changes. It's not giving me the option so perhaps I don't have permission.

[xylar]

Great! Just trying to get CI to pass.

[xylar]

The CI issue is clearly unrelated to this PR. I will look into it when I get back.