Skip to content

Conversation

@milenaveneziani
Copy link
Collaborator

@milenaveneziani milenaveneziani commented Jun 1, 2018

This is very 'work in progress' for now. I just wanted to open this for discussion of some points.

['timeMonthly_avg_normalGMBolusVelocity',
'timeMonthly_avg_vertGMBolusVelocityTop'])
if self.mocAnalysisMemberEnabled:
# I think the bolus velocity should always be included if GM is on
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following should be changed so that it's no longer an option. If GM is on, we need to include the bolus velocity part.

config = self.config

#*********************
# Needs to be edited from here on
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following needs to be edited substantially, but we need to keep the option to read from an already computed climatology. I am still figuring out how I do that, hence no much change can be found below :|

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help here if you can describe more specifically what is needed.

np.amax(latBins)+latBinSize,
latBinSize)
mocTop = self._compute_moc(latBins, nVertLevels, latCell,
regionCellMask, transportZ, velArea)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following 3 lines is the bit to compute the MOC at 26.5. I need to put this into a region loop, keeping in mind that MOC online has two separate arrays: one for the Global bit and one for the regional bit. At the moment, the latter is for the Atlantic, if indRegion=0.

# }}}

def _compute_moc_time_series_analysismember(self): # {{{

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly done, but we probably do not need to save the timeseries to file this time, since it's a super simple calculation once we have the online MOC.
Also, there are lots of repetitions with the _compute_time_series_postprocess function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do make sure to save the time series data. The main reason is that it makes it possible to use it as a reference data set when doing a main vs. ref comparison. In the not-too-distant future, we would want to do the same with the MOC lat vs. depth plots, too, so we will also need to store a copy of them.

self.logger.info(' date: {:04d}-{:02d}'.format(date.year,
date.month))

# hard-wire region=0 (Atlantic) for now
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole region handling will change substantially once we use the regional capability fully.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see #246

depth = ncFile.createVariable('depth', 'f4', ('nz',))
depth.description = 'depth'
depth.units = 'meters'
depth[:] = self.depth
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xylar: I think I took care of most of the changes needed here.
Note the change on the line above depth[:] = self.depth, with respect to the _compute_moc_climo_postprocess respective line. I think we should change it in there too, but I'd like to double check with you first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and made this change in _compute_moc_climo_postprocess as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems fine to me. We might want to think about how this would change for a non-z-level grid in the future but that's a separate issue and there's already a GitHub issue for it (#120)

@milenaveneziani
Copy link
Collaborator Author

I will do some testing now.

@milenaveneziani
Copy link
Collaborator Author

This is not working. It hangs, I think at the climatology task, and no log file is even created.
Looking into it.

@milenaveneziani
Copy link
Collaborator Author

ok, the problem seems to be NERSC. It's having trouble accessing the scratch directory right now. Will try again when things are back to normal.

@milenaveneziani
Copy link
Collaborator Author

ok, after fixing a few things, a test finally gives some reasonable results.
This is the result of running this branch:
http://portal.nersc.gov/project/acme/milena/20180129.DECKv1b_piControl.ne30_oEC.edison_MOConline/ocean/index.html

And this a result I obtained previously using the postprocessing script and accounting for the bolus velocity:
http://portal.nersc.gov/project/acme/milena/20180129.DECKv1b_piControl.ne30_oEC.edison_transects_new/ocean/index.html#moc

Aside from some plot differences (we need to plot better latitude extent), I suppose the differences between these two groups of plots are due to the fact that the online MOC is not accounting for the bolus. I need to test this by looking at a high-res run (no GM).

@milenaveneziani
Copy link
Collaborator Author

@xylar: you will probably hate streamfunctionMOC.py even more now than before :) Lots of repetitions in the code.. But I think it is good to first make sure that things work, and then we can re-structure as much as we want.

latAtlantic = self.lat['Atlantic']
dLat = latAtlantic - 26.5
indlat26 = np.where(dLat == np.amin(np.abs(dLat)))
indlat26 = np.where(np.abs(dLat) == np.amin(np.abs(dLat)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was a bug, and we were just lucky that it worked before.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@milenaveneziani
Copy link
Collaborator Author

One TBD thing is to add a temporary config option to force the use of the postprocessing script, even if the AM is on, until we fix the online MOC to take into account the GM bolus velocity, when GM is on.
I will do this tomorrow.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

From the results, it looks like this is really coming along.

@milenaveneziani
Copy link
Collaborator Author

milenaveneziani commented Jun 5, 2018

Trying to find a high-res case for testing now.

This should be ready to be reviewed @xylar.
@vanroekel, if you have a bit of time, it would be great if you could take a look too.

Any suggestion about how to adjust the latitude range in the plot is welcome. The latbin variable does not depend on the regionName in the MOC AM, it just goes from -90 to 90.

@vanroekel
Copy link
Collaborator

@milenaveneziani I can test this on anvil, you could also test this on the coupled run on theta.

/projects/ClimateEnergy_2/azamatm/E3SM_simulations/20180410.A_WCYCL1950_HR.ne120_oRRS18v3_ICG.theta/

@milenaveneziani
Copy link
Collaborator Author

ok, @vanroekel, thanks.

@milenaveneziani
Copy link
Collaborator Author

@vanroekel or @xylar, could either of you transfer the transect file for the RRS18to6v3 on theta?
I see these two directories where region_masks is:
/projects/ccsm/acme/mpas-analysis_data/mpas_analysis/region_masks owned by Luke
/projects/ccsm/acme/diagnostics_data/region_masks owned by Xylar,
but I don't have permission to write in any of them.

I have completed the MOC online ON test for the coupled high-res run on theta, I only have to do the test for MOC online off. Thanks.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

@milenaveneziani, if you're not it the ccsm group on theta, I think you should get yourself added. I don't want to make /projects/ccsm/acme/diagnostics_data/region_masks world writable. You should definitely be using that directory rather than the one @vanroekel owns on theta. We should delete the other directory, since we don't plan to maintain it.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

I think I just downloaded what you needed (oRRS18to6v3_SingleRegionAtlanticWTransportTransects_masks.nc) from https://web.lcrc.anl.gov/public/e3sm/diagnostics/mpas_analysis/region_masks/
Let me know if that's not right.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

Any suggestion about how to adjust the latitude range in the plot is welcome. The latbin variable does not depend on the regionName in the MOC AM, it just goes from -90 to 90.

@milenaveneziani, I can't think of a better option than having a config option for this. My plan in #246 is to have a separate config section of each region (for now, [streamfunctionMOCGlobal] and [streamfunctionMOCAtlantic]) and these could include both the color map options and the min/max in latitude. If you'd rather just put config options in [streamfunctionMOC] for now, that's okay, too.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

@milenaveneziani, I'm having trouble maintaining a connection to Edison long enough to do a full test. But I did an MOC-only test here:
http://portal.nersc.gov/project/acme/xylar/add_MOC_online/20180129.DECKv1b_piControl.ne30_oEC.edison/ocean/index.html

Everything looks good to me!

ninoSubdirectory = Nino
mhtSubdirectory = MHT


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep 2 blank lines between each section so it's easier to find section. @gstreletz did a lot of work to put in this formatting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry. I thought this was config.default. Ignore my comment above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only removed it because it wasn't consistent in the config example cases: in most cases there weren't blank lines, so I thought of just removing this one.
Instead, config.defaults has the blank lines between each section.

# in its calculation, whereas the postprocessing script does.
# NOTE: this is a temporary option that will be removed once the online
# MOC takes into account the bolus velocity when GM is on.
usePostprocessingScript = True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is perfectly fine as a temporary solution.

@milenaveneziani
Copy link
Collaborator Author

Thanks for downloading the transect file. That is the right one.

depth.description = 'depth'
depth.units = 'meters'
depth[:] = refTopDepth
depth[:] = self.depth
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fine to change this.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

Oh, my goodness! MOC goes from one of the slowest tasks to one of the fastest for runs that can handle it!

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

QU240wLI tests with both python 2 and python 3 ran successfully with the offline MOC (because the analysis member was not enabled).

An EC60to30 test with python 3 ran just fine as well, with the analysis member (essentially same test I tried on Edison with python 2).

An EC60to30wLI test is running now (with python 3 and offline MOC) but I don't think I need to wait for it to approve the PR.

I'd suggest putting in the latitude bounds before merging but I don't think I need to approve the results since it's likely a simple fix. The code to crop the xarray data set should be something like this part of the SOSE transects:
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/ocean/sose_transects.py#L269

        lat = ds.lat.values
        mask = numpy.logical_and(lat >= minLat, lat <= maxLat)
        indices = numpy.argwhere(mask)
        ds = ds.isel(lat=slice(indices[0][0], indices[-1][0]))

@milenaveneziani
Copy link
Collaborator Author

So, these are the results of my test on the coupled high-res case.

Online MOC script ON:
http://portal.nersc.gov/project/acme/milena/20180410.A_WCYCL1950_HR.ne120_oRRS18v3_ICG.theta_MOConlineON/ocean/index.html

Online MOC script OFF:
http://portal.nersc.gov/project/acme/milena/20180410.A_WCYCL1950_HR.ne120_oRRS18v3_ICG.theta_MOConlineOFF/ocean/index.html

I would have preferred a better correspondence for the Atlantic MOC between the 2 cases, but I think this is good enough. At least there is nothing wrong with the script.

Now the only thing to fix is the latitude range.

And yes, now the MOC is the fastest task.

@xylar
Copy link
Collaborator

xylar commented Jun 5, 2018

I would have preferred a better correspondence for the Atlantic MOC between the 2 cases, but I think this is good enough. At least there is nothing wrong with the script.

Global looks great! Atlantic isn't that different and I suspect differenes are due to different masking. The online MOC doesn't use the mask from geometric_features, right? Does it use the exact same southern transect?

@milenaveneziani
Copy link
Collaborator Author

Yes, they use the same transect. I don't remember about the regional mask file. Maybe @vanroekel does.

'latBinMax{}'.format(region))
indLat = np.logical_and(x >= minLat, x <= maxLat)
x = x[indLat]
z = z[:, indLat]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have xarray datasets at this point, so I am subsampling x and z individually.

@milenaveneziani milenaveneziani changed the title WIP: Add script to plot online MOC Add script to plot online MOC Jun 6, 2018
@milenaveneziani
Copy link
Collaborator Author

The latitude bounds as config options work. I just added them to the streamfunctionMOC section for now.

@vanroekel: I think between me and @xylar there was enough testing for this one. I can remove you as reviewer and merge, if that is OK with you.

@vanroekel
Copy link
Collaborator

vanroekel commented Jun 6, 2018 via email

@milenaveneziani
Copy link
Collaborator Author

Thanks Luke.

@milenaveneziani milenaveneziani removed the request for review from vanroekel June 6, 2018 04:20
@milenaveneziani milenaveneziani merged commit e918458 into MPAS-Dev:develop Jun 6, 2018
@milenaveneziani
Copy link
Collaborator Author

This was easier than expected, thanks to @vanroekel notebook script.

@milenaveneziani milenaveneziani deleted the add_MOC_online branch June 6, 2018 04:23
@xylar
Copy link
Collaborator

xylar commented Jun 6, 2018

Congratulations, @milenaveneziani. This seems to have gone quite smoothly.

@xylar xylar mentioned this pull request Jun 6, 2018
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants