Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

splits process_cesm_data() into 2 functions #582

Merged
merged 10 commits into from Nov 24, 2018

Conversation

Projects
None yet
4 participants
@anoukvlug
Copy link
Contributor

anoukvlug commented Oct 25, 2018

This PR is a result of the OGGM hack day. It is meant to contribute to issue #469. This PR is not meant as a final solution, but more as first step and as food to continue the discussion. So don't hesitate to comment on it @fmaussion @nchampollion @sumnonpuella

  • Tests added/passed
  • Fully documented
  • Entry in whats-new.rst

With this PR process_cesm_data() is split into 2 functions. prepro_cesm_data and process_gcm_data(). process_gcm_data is meant to make it easier to run OGGM with the climate of other GCM's. prepro_cesm_data replaces process_cesm_data(). prepro_cesm_data uses the process_gcm_data() to generate the climate files for OGGM. It also can be used as an example on how to use other GCM's as input.

  • prepro_cesm_data is placed is a new file. I am not sure what the best name and location for this file would be.
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Oct 25, 2018

Hello @anoukvlug! Thanks for updating the PR.

Comment last updated on November 14, 2018 at 12:47 Hours UTC
@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Oct 27, 2018

Thanks @anoukvlug ! I like the gist of it, this is a good idea! But it needs a couple corrections, also for the tests to pass.

oggm/core/climate_prepro.py is a good place!

@sumnonpuella

This comment has been minimized.

Copy link
Contributor

sumnonpuella commented Oct 28, 2018

@anoukvlug I like the idea a lot! Do you see then see other GCM prepro functions being added to climate_prepro.py? That might help keep climate.py clean. Once this is merged, I might copy the structure for the process_equil_ccsm function.

@anoukvlug

This comment has been minimized.

Copy link
Contributor Author

anoukvlug commented Nov 13, 2018

@fmaussion and @sumnonpuella thank you for your comments.

@sumnonpuella the idea is indeed that other GCM prepro functions can be added to the climate_prepro.py. I am happy to hear that you might be interested in using this structure in a later stage :)

@anoukvlug anoukvlug force-pushed the anoukvlug:split-process-cesm branch from d202942 to 8410609 Nov 14, 2018

@fmaussion
Copy link
Member

fmaussion left a comment

Thanks! Looking very good. I have a few small comments, and one last request: can you do a project-wide search for "cesm_data" in the codebase? I believe there are some references in the docs that need to be updated as well.

Then it should be good!!!

Parameters
Parameters

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

Remove indent here

@entity_task(log, writes=['gcm_data'])
def process_gcm_data(gdir, filesuffix='', precp=None, temp=None,
time_unit=None, time2=None):
''' Applies the anomaly method to the climate data and stores the data in a

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

Use """ instead of ''' (for consistency)

from Otto-Bliesner et al. (2016), to the high-resolution CL2 climatologies
(provided with OGGM) and writes everything to a NetCDF file.
@entity_task(log, writes=['gcm_data'])
def process_gcm_data(gdir, filesuffix='', precp=None, temp=None,

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

For consistency with the rest of the code, can you call this prcp instead of precp?

year (time) int64
time_unit : str
format: days since e.g.: days since 0850-01-01 00:00:00
time2 : numpy.ndarray

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

what's time2 here? I think another name and doc would help. Maybe we can even don't use it and extract it from the dataarrays?

This comment has been minimized.

Copy link
@anoukvlug

anoukvlug Nov 15, 2018

Author Contributor

I agree that it is very unclear in this way, I include the doc and another name. I was thinking maybe time_dates would be better.

time2 is the variable time that is needed in the write_monthly_climate_file(). I agree that it would be better if the time could be extracted from the dataarrays. I only don't know yet how to include it.

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 15, 2018

Member

OK. You can give it a try and if not I'll have a look

for an example see: climate_prepro.process_cesm_data()
'''

This comment has been minimized.

Copy link
@fmaussion
filesuffix : str
append a suffix to the filename (useful for ensemble experiments).
fpath_temp : str
path to the temp file (default: cfg.PATHS['gcm_temp_file'])

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

Here (and below) I guess we should change if for cfg.PATHS['cesm_temp_file'] . What do you think?

time_unit = time1.units

return process_gcm_data(gdir, filesuffix=filesuffix, precp=precp,
temp=temp, time_unit=time_unit, time2=time2)

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

I don't think we should return anything here (process_gcm_data doesn't have a return values).

In all cases, you should not return from the function before closing the files. I would recommend to shift the lines below to above, and the last call of the function should be process_gcm_data(...)

@@ -21,7 +21,7 @@
from oggm.core.climate import process_cru_data
from oggm.core.climate import process_histalp_data
from oggm.core.climate import process_custom_climate_data
from oggm.core.climate import process_cesm_data
from oggm.core.climate import process_gcm_data

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 14, 2018

Member

here you can add your new task as well: from oggm.core.climate_prepro import process_cesm_data

@anoukvlug

This comment has been minimized.

Copy link
Contributor Author

anoukvlug commented Nov 14, 2018

@fmaussion thanks for doing the review so fast :) I agree the changes you suggest and just implemented some of them. The remaining part I will try to include later this week.

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 15, 2018

@anoukvlug this looks great! Are you still working on it? If not I'd like to go over it one last time tomorrow morning and merge it ;-)

@anoukvlug

This comment has been minimized.

Copy link
Contributor Author

anoukvlug commented Nov 16, 2018

Thank you @fmaussion!

I am not working on the PR anymore. Though I am not sure if it should be merged already. I did some renaming in this PR in anticipation of PR #580, that like this PR also addresses issue #567. I thought about adjusting my PR to PR #580 once it is merged. What do you think?

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 23, 2018

@anoukvlug regarding #580 : as I said. I think we should close #580 and start it again from a cleaner state, i.e. once your PR is merged.

I just made some edits to your code, mostly simplifications regarding time handling which are now possible since xarray v0.11. The code is much cleaner and shorter, and I believe we are on a very good track.

Let's see if the test still pass ;-)

@anoukvlug

This comment has been minimized.

Copy link
Contributor Author

anoukvlug commented Nov 23, 2018

@fmaussion thank you for making the changes.

I am wondering if the failing test have something to do with the timestamp issue #157. I think the reason for not using:
ts_tmp_avg = temp.sel(time=slice('1961', '1990'))
before in PR #188 was related to that issue.

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 24, 2018

I am wondering if the failing test have something to do with the timestamp issue

Yes that's correct! But recent xarray updates have now finally made it possible to handle non-conventional calendars the same way as conventional ones. You'll need xarray v0.11 for this to work

@fmaussion fmaussion merged commit 73a77d8 into OGGM:master Nov 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+10.0%) to 86.257%
Details
@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 24, 2018

Thanks Anouk this is really a great step towards easy use of GCM data!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.