-
Notifications
You must be signed in to change notification settings - Fork 25
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
IceNet demonstrator #6
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:10Z
Aside from my minor comments, my main comment at this point in the review is that the notebook is super long. Personally, I prefer shorter notebooks that have a good amount of markdown text and the code cells are not too long. The notebook has become this long because of copying code directly from my codebase, which has additional content necessary for making the scripts generic to various model/computations. But in this case we should exploit two things to reduce length: 1) we know we only want to forecast/analyse with IceNet and what metrics we want, 2) we know the notebook will only be run once, linearly from start to finish.
People lose attention easily so let's try make it as short as we can! acocac commented on 2021-09-24T16:00:08Z The word 'excels' is not correct in IceNet excels the range of accurate sea ice forecasts. You could use 'advances' (as we do in the paper abstract) or a synonym! Done. IceNet advances...
It should be 'to produce seasonal Arctic sea ice forecasts' Done.
It is unclear why you use ERA5 data going back to April 2016 if only forecasting 2020 & Perhaps mention in 'Highlights' that you download sample data from a Zenodo repository? Done. * Forecast a single year, 2020, from Apr 2019 to Dec 2020 analysis-ready data dowloaded from a Zenodo repository.
Can you frame this as a 'demonstrator of the IceNet forecasting system' or similar Done. The purpose now frames it: Demonstrate IceNet, a deep learning sea ice forecasting system trained using climate simulations and observational data.
Tell people that at the end of the notebook there will be some interactive visualisations, so people should 'make sure to get to the end!' or something :-) The last two items of the highlights: * Visualise IceNet’s seasonal ice edge predictions at 4- to 1-month lead times. * Interactive plots comparing IceNet predictions against ECMWF SEAS5 physics-based sea ice probability forecast and linear trend statistical benchmark. tom-andersson commented on 2021-10-19T15:56:19Z
acocac commented on 2021-10-20T19:21:37Z
|
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:11Z Is there a way to remove the blank bokeh(?) outputs below this import cell? acocac commented on 2021-09-24T10:35:03Z I am not sure if they can be removed. Why do you suggest to remove it? |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:12Z Line #5. network_exist = [f for f in target_networks if os.path.isfile(f'./networks/network_tempscaled_{f}.h5')];
acocac commented on 2021-09-24T16:14:19Z removed to make the cell shorter...
url = 'https://ramadda.data.bas.ac.uk/repository/entry/get/'
|
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:12Z Line #11. if len(network_non_exist) > 0: This reads strangely... If there are 1 or more networks that don't exist, do the download? Is there a variable name typo? acocac commented on 2021-09-24T10:44:12Z Make the step simple by removing network_exist and network_non_exist vars
url = 'https://ramadda.data.bas.ac.uk/repository/entry/get/' |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:13Z Should be 'ERA5' in title acocac commented on 2021-09-24T10:44:31Z done, thanks! |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:14Z Line #1. # very slow > plan B: to provide the analysis-ready file, siconca_EASE.nc Can delete comment now :-) acocac commented on 2021-09-24T12:25:07Z done |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:14Z perhaps 'data loader configuration file'? acocac commented on 2021-09-24T12:25:44Z change to Download data loader configuration file |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:15Z Perhaps you could load the file with json and print a snippet? But no worries if it prints too much stuff.
e.g.
open(dataloader_config_fpath, 'r') as readfile: dataloader_config = json.load(readfile) acocac commented on 2021-10-18T10:28:27Z done |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:15Z Line #9. ensemble_seeds_and_mean = ensemble_seeds.copy() Delete L9 and L10 and move to DataArray setup cell below where it is used? acocac commented on 2021-09-24T12:44:37Z done |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:16Z
acocac commented on 2021-10-18T10:41:43Z
|
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:17Z Line #5. Add ensemble_seeds_and_mean = ensemble_seeds.copy() |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:17Z Line #35. coords['seed'] = ensemble_seeds_and_mean Add these bits to the lines above where the coords dictionary is instantiated acocac commented on 2021-10-18T13:58:33Z done!
|
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:18Z Line #37. shape = (len(ensemble_seeds_and_mean), *shape, 3) Replace acocac commented on 2021-09-24T12:48:06Z which raw tuple? |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:18Z Line #39. model_forecast = xr.DataArray( Would it be good to |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:19Z Can you explain how the ensemble mean prediction of the three chosen ensemble members is computed by averaging the outputs? Can you state how the predictions of P(SIC<15%), P(15%<SIC<80%), P(SIC>80%) is converted to the 'sea ice probability', P(SIC>15%), by summing the latter two classes? |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:19Z Line #1. start_date = all_start_dates[0] Remove acocac commented on 2021-09-24T12:53:49Z done |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:20Z Line #44. del(model_forecast) ## remove model_forecast from the memory Apparently we should call acocac commented on 2021-10-18T14:00:43Z it doesn't seem to be longer necessary as we're doing a small computation. |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:21Z Finish markdown cell - explain what metrics we are computing to analyse forecast performance acocac commented on 2021-10-18T14:19:31Z Added: To analyse the forecast performance, IceNet's researchers compute two metrics, |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:21Z Line #11. icenet_ID = 'unet_tempscale' Sorry I think this should be acocac commented on 2021-10-18T14:20:59Z the new notebook contains a simpler notation for the model, IceNet |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:22Z Can you quickly explain how dask chunking works here or in a code comment? acocac commented on 2021-10-18T14:21:23Z Not longer necessary as the computations are in the memory. |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:22Z Line #11. if icenet_ID in model_compute_list: I guess we don't need this since the point of the notebook is to analyse IceNet forecasts? acocac commented on 2021-09-24T13:14:17Z removed |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:23Z Needs a markdown cell for more context about how we are downloading all the pre-computed results from the Nature Communications paper from the Polar Data Centre. acocac commented on 2021-10-18T14:24:37Z Added: It is worth to mention other pre-computed results from the Nature Communications paper can be downloaded including output results table, uncertainty, netCDF forecast of the 25 ensemble members, among others. |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-09-23T16:16:23Z This cell is very long... acocac commented on 2021-10-18T14:25:46Z the cell is considerably shorter in the new version.
now = pd.Timestamp.now() new_results_df_fname = now.strftime('%Y_%m_%d_%H%M%S_forecast_results.csv') new_results_df_fpath = os.path.join(config['forecast_results_folder'], new_results_df_fname) |
Thanks for all this @acocac! notebooksharing.space is a nice option to view the plots, although unfortunately the slides do not work. I'll stick to using the .zip files you sent. I'm taking a look at ReviewNB now. |
View entire conversation on ReviewNB |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-10-19T16:14:47Z
|
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-10-19T16:14:48Z
|
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-10-19T16:14:48Z Line #6. month_slider = pn.widgets.DiscreteSlider(name="Month", options=month_name, value='September') If would be cool if you could make the slider say '<month> 2020' so that the actual time is known. |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-10-19T16:14:49Z Can you increase the label and ticks fontsize so they are basically equal to the notebook markdown fontsize? acocac commented on 2021-10-20T21:05:43Z - font size increased. |
View / edit / reply to this conversation on ReviewNB tom-andersson commented on 2021-10-19T16:14:50Z
|
View entire conversation on ReviewNB |
- font size increased. View entire conversation on ReviewNB |
@tom-andersson, thanks for the great feedback. It has been very interesting to me to reproduce the IceNet paper. I've worked on most of the latest suggestion including:
I'll close the issue once I confirm the IceNet notebook of the Thanks!! ❄️ |
The reviewer has recommended publication. Some future work is needed to improve text. |
Nick @nbarlowATI suggested some changes for the IceNet demo (see PR#6 in the Turing Pangeo Examples repository). I incorporated them and added Nick as reviewer. |
A PR for contributing a IceNet Sea Ice Forecasting demonstrator to the Environmental AI book. The notebook:
Notes:
The notebook maintains the project structure of the IceNet paper repository. The mask data are generated by
gen_masks.py
script. The remaining sample data and configuration file are downloaded from a public zenodo repository.I have added Tensorflow 2.2.0 to the environment.yml file.