Adds SSS model vs obs capability #55

vanroekel · 2016-12-08T15:40:09Z

This adds a comparison to Aquarius monthly observations. This has been tested against beta0 in the updateTimeseries branch. This is a clean PR against develop. I retested against an alpha8 run and things appear to be working. My config.analysis for testing on edison is at /global/homes/l/lvroekel/config.analysis_SSS_edisontest

vanroekel · 2016-12-08T15:41:03Z

@milenaveneziani In testing this PR, there was an issue using the Aquarius data in the project space on edison. Could you check permissions on that file?

xylar · 2016-12-08T15:45:48Z

@vanroekel and @milenaveneziani, this branch will need to be updated to use variable and stream maps like in #52. Depending on how you want to handle the merges, we would either want to update this branch after #52 gets merged or merge this branch and then update #52 to handle these additions.

Let me know which it will be.

vanroekel · 2016-12-08T15:49:43Z

@xylar and @milenaveneziani , given that this PR is a fairly quick change, I would say to merge #52 and then I'll update here.

milenaveneziani · 2016-12-08T15:55:20Z

good catch @vanroekel: I now fixed the permissions for the Aquarius data file.

pwolfram · 2016-12-09T17:45:09Z

@vanroekel, I am happy to review this once it is rebased following completion of other PRs if that is helpful.

xylar · 2016-12-09T18:18:16Z

@vanroekel and @pwolfram, note that there will be conflicts and additional development required, so it's not going to be a simple rebase. There are 2 options, and I'm fine with either:

@vanroekel does the rebase and we use this as a chance to find out if the new variable map capability (and some of the increased modularization I did in ocean/modelvsobs.py) is intuitive or not or
I do the rebase and fix things up the same way I did for mld and sst, which will likely make things go faster.

Why don't we see how it goes with 1 and I'll bail you out if it doesn't go well.

pwolfram · 2016-12-09T18:55:59Z

@xylar and @vanroekel, I'm happy to contribute here too so that we can spread the work / knowledge around if that is helpful.

vanroekel · 2016-12-09T20:02:16Z

@xylar and @pwolfram Let me give this a go and if I get stuck I'll certainly reach out to one or both of you. I think though I may close this PR and open a new one with the updates given comments from @xylar. Does this sound reasonable?

pwolfram · 2016-12-09T20:04:43Z

@vanroekel, why would you close this PR and open up a new one? It sounds like the purpose of this PR should be the same so I don't think this is necessary but I could be missing something.

vanroekel · 2016-12-09T20:07:25Z

Okay. I imagined the code being so different as to require a new PR, but I see what you are saying about the actual purpose of the PR being the same so keeping it. Thanks for the clarification.

vanroekel · 2016-12-09T20:14:14Z

@pwolfram @xylar @milenaveneziani should I attempt the rebase for this PR after #52 is merged or wait for others as well?

pwolfram · 2016-12-09T20:17:28Z

@vanroekel, my understanding is that #48 needs merged, then #52. This code will obviously need functionality in both merges, so my intuition is to wait for now but @xylar and @milenaveneziani should correct me if I'm wrong.

xylar · 2016-12-09T20:26:14Z

@vanroekel, you have a few options:

You could try rebasing onto Map variables and streams from MPAS dycore names to MPAS-Analysis names #52 now, with the understanding that you'll have to re-rebase later once Fix time-series scripts use time bounds properly #48 and Map variables and streams from MPAS dycore names to MPAS-Analysis names #52 have been merged (though there's a small chance this wouldn't be necessary). I think this would be reasonable to do, since I don't expect major changes in either branch. (Note that Map variables and streams from MPAS dycore names to MPAS-Analysis names #52 has been branched off of Fix time-series scripts use time bounds properly #48, so both will be incorporated.)
You could wait until Map variables and streams from MPAS dycore names to MPAS-Analysis names #52 has been merged (and Fix time-series scripts use time bounds properly #48 before it) to make sure there's smooth sailing. This is likely to be somewhat less work but probably less time efficient, too.

I'm generally impatient and want to get as much done as possible so I'd personally choose 1. That doesn't really mean it's the smarter option, though.

There should be no other PRs you need to wait for after #52 before updating this PR. No surprises in the works.

pwolfram · 2016-12-12T15:13:11Z

@vanroekel, #48 and #52 have now been merged so we can directly move forward with this PR following a rebase.

vanroekel · 2016-12-12T15:17:22Z

Thanks @pwolfram, working on this now.

vanroekel · 2016-12-12T15:19:58Z

@xylar and @pwolfram in working on the update here, I have a question. I see that

obs_filename = "%s/Aquarius_V3_SSS_Monthly.nc" % obsdir

should change to

obs_filename = "{}/Aquarius_V3_SSS_Monthly.nc".format(obsdir)

In order to follow the changes in #52. Could I ask the advantage of one over the other?

pwolfram · 2016-12-12T15:29:03Z

@vanroekel, please see https://pyformat.info/. Note, we need this for #43

xylar · 2016-12-12T15:30:06Z

@vanroekel, my understanding is that the % notation is not supported (or at least strongly discouraged) in python 3, which we would like to move toward supporting. Also, The tools I use for checking PEP8 compliance think % a modulo operator and require spaces around it. Given that I as editing string formatting anyway, I decided to use the preferred notation:

https://docs.python.org/3.1/library/string.html#format-examples

This adds a comparison of model data against Aquarius monthly output (2011 - 2014).

vanroekel · 2016-12-12T15:57:50Z

@xylar and @pwolfram thanks for the excellent explanations. It certainly seems like the new .format is much more extensible in addition to the python 3 compliance.

@pwolfram and @milenaveneziani , this branch should be ready to review again. I've tested against alpha8 and beta0. Annual SSS from alpha8 is

Annual SSS from beta0 (same time periods)

The config.analysis script I used for the tests is at /global/homes/l/lvroekel/config.analysis_SSS_edisontest

pwolfram

@vanroekel, thanks for the clean PR. I have a few very minor comments to clean it up further but overall the code looks good.

The biggest comment is that it would be good, if possible, to somehow reproduce the xarray / dask error so that we can submit an issue and get that weird bug fixed. However, this obviously isn't a high-priority.

pwolfram · 2016-12-12T16:16:15Z

config.analysis

+cmapModelObs = RdYlBu_r
+# colormap for differences
+#cmapDiff = RdBu_r
+cmapDiff = coolwarm


@vanroekel, do we really need all the commented lines above? The same is true below too...

My recommendation would be to keep this file as clean as possible by only having comments as absolutely necessary to help avoid confusion in the long term.

@pwolfram and @vanroekel, I agree that I don't love having these commented out options in the default config file. Ideally comments should be for clarification rather than alternative options, and instead we would have alternative config files or scripts for modifying config files or something along those lines for changing color maps, etc.

But I also know that this convention has been used for other modelvsobs entries so it's probably not something to address in this specific case but rather to clean up more broadly later on.

Sounds good to me.

pwolfram · 2016-12-12T16:17:02Z

mpas_analysis/ocean/ocean_modelvsobs.py

+
+        # The following line converts from DASK to numpy to supress an odd
+        # warning that doesn't influence the figure output
+        ds_tslice.SSS.values


Eventually we will be able to remove this line, most likely.

It would be good to submit an xarray / dask bug report if we could get a simple xarray or dask - only reproducible manifestation of this error if possible. However, this may be time consuming and not worth it, at least in the short-term.

@pwolfram not sure if this is helpful information, but I can also suppress this warning if I open the SSS observations with xr.open_dataset instead of xr.open_mfdataset. It's curious that the multiple file version of open throws a warning for SSS, but not for SST or MLD.

I think it is some type of dask-xarray integration error. Getting to the bottom of it will potentially be tricky. I'd say pass on this for now.

pwolfram · 2016-12-12T16:18:38Z

mpas_analysis/ocean/variable_stream_map.py

@@ -20,7 +20,7 @@
 oceanVariableMap['avgLayerTemperature'] = \
    ['time_avg_avgValueWithinOceanLayerRegion_avgLayerTemperature',
     'time_avg_avgValueWithinOceanLayerRegion_avgLayerTemperature_1',
-    'timeMonthly_avg_avgValueWithinOceanLayerRegion_avgLayerTemperature']
+     'timeMonthly_avg_avgValueWithinOceanLayerRegion_avgLayerTemperature']


Ditto for me too!

xylar · 2016-12-12T17:11:22Z

@vanroekel, looks good to me. As you probably noticed, it's a little weird that we're mapping variables time_avg_activeTracers_salinity, etc. to sss and similarly for temperature. It might make more sense to have a dictionary inside ocean_modelvsobs.py that maps from field to another variable name that is more relevant to what is being loaded from time-series stats. So, say sss would point to salinity or timeAvgSalinity or something like that that is more accurate (and more reusable in cases where we might not just want the surface value).

Anyway, I think this should be addressed with a separate PR that also fixes what I did for sst in #52. I'm just mentioning it here so we can make an issue about it once this PR gets merged and it get forgotten.

xylar · 2016-12-12T17:11:35Z

@milenaveneziani, this is ready to merge as far as I'm concerned.

xylar assigned milenaveneziani Dec 8, 2016

xylar added the enhancement label Dec 8, 2016

pwolfram mentioned this pull request Dec 9, 2016

Summary of key analysis tasks #32

Closed

22 tasks

pwolfram mentioned this pull request Dec 12, 2016

Update submodule in PreAndPostProcessingScripts following support for reading namelist and streams files #28

Closed

4 tasks

Adds SSS model vs obs capability

998b5ca

This adds a comparison of model data against Aquarius monthly output (2011 - 2014).

vanroekel force-pushed the SSSmodelvsobs branch from 7ae2eda to 998b5ca Compare December 12, 2016 15:46

Fixes pep8 issues

d8f2b60

pwolfram reviewed Dec 12, 2016

View reviewed changes

milenaveneziani merged commit d8f2b60 into MPAS-Dev:develop Dec 12, 2016

Adds SSS model vs obs capability #55

Adds SSS model vs obs capability #55

Uh oh!

Conversation

vanroekel commented Dec 8, 2016

Uh oh!

vanroekel commented Dec 8, 2016

Uh oh!

xylar commented Dec 8, 2016

Uh oh!

vanroekel commented Dec 8, 2016

Uh oh!

milenaveneziani commented Dec 8, 2016

Uh oh!

pwolfram commented Dec 9, 2016

Uh oh!

xylar commented Dec 9, 2016

Uh oh!

pwolfram commented Dec 9, 2016

Uh oh!

vanroekel commented Dec 9, 2016

Uh oh!

pwolfram commented Dec 9, 2016

Uh oh!

vanroekel commented Dec 9, 2016

Uh oh!

vanroekel commented Dec 9, 2016

Uh oh!

pwolfram commented Dec 9, 2016

Uh oh!

xylar commented Dec 9, 2016

Uh oh!

pwolfram commented Dec 12, 2016

Uh oh!

vanroekel commented Dec 12, 2016

Uh oh!

vanroekel commented Dec 12, 2016

Uh oh!

pwolfram commented Dec 12, 2016

Uh oh!

xylar commented Dec 12, 2016

Uh oh!

vanroekel commented Dec 12, 2016

Uh oh!

pwolfram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Dec 12, 2016

Uh oh!

xylar commented Dec 12, 2016

Uh oh!

Uh oh!