MetOffice / PyPRECIS

PyPRECIS is the python based training environment for Met Office PRECIS training courses.
BSD 3-Clause "New" or "Revised" License
20 stars 2 forks source link

add in improvements for review #115 #128

Closed nhsavage closed 2 years ago

nhsavage commented 2 years ago

response to review:

Review of sections 1.1-1.2 (User testing)

L275: Just checking that the latest version moves up ONE level, not two (into data_v2).

fixed L315: Python & Iris logo doesn't load Works for me. This is probably due to me just sending you the notebook - the images are in the img folder I didn't send

L323: (e.g. air_temperature). Typo: loading

Fixed

L328: broken link. Replace with https://scitools-iris.readthedocs.io/en/stable/userguide/iris_cubes.html

Pointed at final 2.x version as not using Iris 3 yet

L389: image of a cube doesn't load

Works for me - see above

L403: Replace "answer" with "Double click here to type your answer"

done

L431: plotting throws 2 UserWarnings: grid lat/lon is not bounded, guessing contiguous bounds. Can bounds be added, or the warning message suppressed?

done

L431: It'd be nice to replace the omitted question / comment about the plot to end this section. e.g. "Sense check the data plotted: Can you make out sections of the East Asian coastline? How about the scale? As we progress through these workbooks, we'll learn how to process the data into more intuitive units and mask / add country boundaries, so it's easier to understand the information."

Done

Section 1.3 Merge Problems

L443: Add section number (1.3) to title

Done L445: typo "sequential" Done L455: links to an old version of Iris. Replace link with latest stable vn: https://scitools-iris.readthedocs.io/en/stable/userguide/loading_iris_cubes.html Intentional as this is the version the notebooks use at present L463: Reference to 'West Asia' domain. Do you mean EAST Asia? If not, the CORDEX West Asia domain appears to have been renamed 'South Asia'. Can we follow suit? Also, hyperlink to the domain info? https://cordex.org/domains/region-6-south-asia-2/ New text: "some temperature data from the West Asia CORDEX Domain". fixed L484: Make it more explicit that the earner is expected to edit the code block. e.g. "Complete the print statement to see which East Asian files match the specified criteria." Done L503: Add a question: "How many files were returned?" (Answer: 3) Done L567: Typo: differences fixed L576: links to vn2.4 of Iris. Perhaps replace with this link? https://scitools-iris.readthedocs.io/en/stable/generated/api/iris/util.html#iris.util.equalise_attributes See above

· Section 1.1 helpfully labels the steps (i.e. 1.1 a-g). Please could this be done in other sections as well? e.g. L339: "a) First, ..." It'd be helpful if teaching in a classroom / referring back to sections in later questions. e.g. L339: a) First, … Done · L417: replace i)with b) Done · L586: Error: Exception: The function "iris.experimental.equalise_cubes.equalise_attributes" has been moved. Please replace "iris.experimental.equalise_cubes.equalise_attributes()" with "iris.util.equalise_attributes()". (Actioned replacement to continue testing) This is due to you using scitools/default - this works ok at 2.4 as used here (I should have told you to install the kernel) · L675: Always nice to end a section with a reflective question on what has been achieved. e.g. Now that we have combined multiple files into a single cube, what is the cube's shape? How does this compare with the cube_list created in 1.2e? Based on all the information you've gained about the data so far, what time period do you expect the data in this cube to span?(ANSWER: Jan 1981 - Dec 2005, noting this is only from the filenames, which could be misdirecting) Done · L681: Add 1.4to section title (Extracting Data) Done · L683: How about making this an interactive step? "This is a lot of data, so we will cut this down using a time constraint. We only want the cube to include data from December 1989 to November 1991 (inclusive). Edit the code below to specify the missing end date. (Hint: specify the adjacent months BEFORE and AFTER the time period you wish to keep.)" Done · L698-699: time_constraint = iris.Constraint(time=lambda cell: PartialDateTime(year=1989,month=11) \n, < cell.point < PartialDateTime(year=YYYY, month=MM))\n L703: (split code block into two parts) "Check the first and last timesteps in your constrained cube are correct:" Done · L714: Add 1.5to section title (Saving Data) Done · L716: save DONE · L718: Rephrase Good use of filenameas Well-written filenames? I went with "well chosen" · L730: save_locationoutputs a double slash: .//cordex_training. Omit extra /on this line Done · L744: Grammar? As we progress through these worksheets, keep a note of how we update the file names when making further changes to the data. · Done

· L798: Broken Iris image. · See above · L799: Update copyright year from 2019. Done in all notebooks

nhsavage commented 2 years ago

on the instructions – that will be platform dependent and be included in joining instructions.

I’ll fix the other part and merge.

nhsavage commented 2 years ago

on the instructions – that will be platform dependent and be included in joining instructions.

I’ll fix the other part and merge.

N

From: Rosanna @.> Sent: 07 April 2022 10:58 To: MetOffice/PyPRECIS @.> Cc: Savage, Nicholas @.>; Author @.> Subject: Re: [MetOffice/PyPRECIS] add in improvements for review #115 (PR #128)

This email was received from an external source. Always check sender details, links & attachments.

This looks great to me.

· L586: Error: Exception: The function "iris.experimental.equalise_cubes.equalise_attributes" has been moved. Please replace "iris.experimental.equalise_cubes.equalise_attributes()" with "iris.util.equalise_attributes()". (Actioned replacement to continue testing) This is due to you using scitools/default - this works ok at 2.4 as used here (I should have told you to install the kernel) Should this be specified anywhere for learners, or is it a default somewhere?

Section 1.5 could do with labelling a) and b) for consistency.

Then I think you're done :)

— Reply to this email directly, view it on GitHubhttps://github.com/MetOffice/PyPRECIS/pull/128#issuecomment-1091459026, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABPAHXRLUSHQSWWKITKNQFDVD2WRBANCNFSM5RUQZQLA. You are receiving this because you authored the thread.Message ID: @.**@.>>