1623 add training #3145
1623 add training #3145
Conversation
…amples fully works.
|
@arporter , I have addressed all changes. I also added a lot of additional tests, since I had at least two examples being broken that were not picked up by my testing - which was pretty bad in the actual training sessions. One issue is fixed already (and also properly tested here), one is still outstanding (#3252). The fact that this tutorial contains additional important testing makes me push for getting this in sooner than later. Otherwise we risk a never-ending PR: as PSyclone gets updated and the training is not tested, there is a chance that something breaks the training, Then I need to debug and fix this (or get help to fix this), instead of these problems being flagged when the PSyclone improvements are added (and then being fixed by the original developer before the PR is merged). Note that I have opened #3270 for additional training improvements (e.g. linking files instead of duplicating ... at least evaluating this option; adding syntax highlighting). And also #3208 to review the individual training sections. While I totally agree that this is not ideal, my suggestion would be that the review here focuses just on the testing side-of things, to get any issues there sorted out. No one has the time to proof-read all of this text material, and as mentioned, delays this PR just mean this will keep on being more work to keep this up-to-date. Yes, there is a risk that it might take a long time before this material (as in: the READMEs) gets properly updated, and I understand your reluctance. I am open for any suggestion you have, I don't have a particular brilliant idea either :( Am triggering the full CI as well. |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
I'm enjoying going through (in outline) your game-of-life example - it's great to see that you can recover performance :-)
Since most of the example transformation scripts need their module and apply-method docstrings updating, I'm going to pause here to let you do that.
As you suggested, I'm basically ignoring the actual text of the readme's so I am by no means reviewing the actual tutorials.
| # Author: J. Henrichs, Bureau of Meteorology | ||
|
|
||
| # The transform target `test` runs tests on the gitlab CI (i.e. without | ||
| # compilation), `test_run` will compile all binaries (as far as possible |
There was a problem hiding this comment.
Do you mean compile instead of test_run and then s/run/test_run/ on the following line?
| .PHONY: allclean clean compile test test_run | ||
|
|
||
| allclean clean compile test test_run: | ||
| $(MAKE) -C psyclone_for_lfric_users $@ |
There was a problem hiding this comment.
Ah, so this part is for LFRic users rather than developers? I think "lfric_users" would be fine since this is already under the "training" directory in the PSyclone repository.
There was a problem hiding this comment.
If I do:
cd PSyclone/tutorial/training
make compilethen I get:
make -C psyclone_for_lfric_users compile
make[1]: Entering directory '/home/me/Projects/PSyclone/tutorial/training/psyclone_for_lfric_users'
make[1]: *** No rule to make target 'compile'. Stop.
tutorial/training/README.rst
Outdated
| This is the training material for the usage of PSyclone, a code | ||
| transformation and generator tool. PSyclone was originally developed | ||
| for the UK Met Office’s new LFRic numerical weather prediction system, | ||
| but it’s features of modifying large codes based on scripts have found |
| @@ -0,0 +1,90 @@ | |||
| PSyclone Training | |||
There was a problem hiding this comment.
Thanks for adding this Joerg - it's very helpful. Please could you also add a brief README in the directory above to explain what is contained in each of the subdirectories - i.e. it's not clear what the difference between 'training' and 'tutorial' is. Ideally, at some point, we'll probably want to merge the two.
tutorial/training/README.rst
Outdated
| more open ended, and trainees are encouraged to go back to these | ||
| examples and implement better solutions later. | ||
|
|
||
| The trainings material is split into four parts, and the files |
| # ----------------------------------------------------------------------------- | ||
| # Author: J. Henrichs, Bureau of Meteorology | ||
|
|
||
| '''Python script intended to be passed to PSyclone's generate() |
|
|
||
| def trans(psyir): | ||
| ''' | ||
| Take the supplied psyir object, and fuse all loops. |
|
|
||
| def trans(psyir): | ||
| ''' | ||
| Take the supplied psyir object, and fuse the last three loops. |
| # ----------------------------------------------------------------------------- | ||
| # Author: J. Henrichs, Bureau of Meteorology | ||
|
|
||
| '''Python script intended to be passed to PSyclone's generate() |
|
|
||
| line_count = 0 | ||
|
|
||
| do |
Adds the PSyclone training material (based on #3131)