From ae6fe0ee9b4cefc9bd1337e943c9ff853ececd91 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Thu, 10 Aug 2023 15:26:27 +0100 Subject: [PATCH 01/22] add case with new cmd line arg --- tests/integration/test_analysis_compare_end-end.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/integration/test_analysis_compare_end-end.py b/tests/integration/test_analysis_compare_end-end.py index 2845c7da..471b2ed0 100644 --- a/tests/integration/test_analysis_compare_end-end.py +++ b/tests/integration/test_analysis_compare_end-end.py @@ -49,3 +49,14 @@ def test_run_analysis_compare(): bgcval2_test_data() with arguments('analysis_compare', '-y', 'input_yml/integration_testing.yml'): main() + + +@patch('bgcval2.analysis_compare.main', new=wrapper(bgcval2.analysis_compare.main)) +@pytest.mark.usefixtures('test_create_files') +def test_run_analysis_compare_with_skip_timeseries(): + """Patch and run the whole thing.""" + bgcval2_test_data() + with arguments('analysis_compare', '-y', + 'input_yml/integration_testing.yml', + '--skip_timeseries'): + main() From d576930551ad62072263905e02ad21dcfd4271ab Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Thu, 10 Aug 2023 16:25:52 +0100 Subject: [PATCH 02/22] run test with no-skip too --- tests/integration/test_analysis_compare_end-end.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/integration/test_analysis_compare_end-end.py b/tests/integration/test_analysis_compare_end-end.py index 471b2ed0..1f97ed6b 100644 --- a/tests/integration/test_analysis_compare_end-end.py +++ b/tests/integration/test_analysis_compare_end-end.py @@ -60,3 +60,14 @@ def test_run_analysis_compare_with_skip_timeseries(): 'input_yml/integration_testing.yml', '--skip_timeseries'): main() + + +@patch('bgcval2.analysis_compare.main', new=wrapper(bgcval2.analysis_compare.main)) +@pytest.mark.usefixtures('test_create_files') +def test_run_analysis_compare_with_no_skip_timeseries(): + """Patch and run the whole thing.""" + bgcval2_test_data() + with arguments('analysis_compare', '-y', + 'input_yml/integration_testing.yml', + '--no-skip_timeseries'): + main() From c79c0454f367f43b8084a01a4816d4c4fd5e94db Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 10 Aug 2023 16:28:03 +0100 Subject: [PATCH 03/22] Update bgcval2/analysis_compare.py Co-authored-by: Valeriu Predoi --- bgcval2/analysis_compare.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgcval2/analysis_compare.py b/bgcval2/analysis_compare.py index 56217169..e1c3be68 100755 --- a/bgcval2/analysis_compare.py +++ b/bgcval2/analysis_compare.py @@ -556,7 +556,7 @@ def load_yml_and_run(compare_yml, config_user, skip_timeseries): do_mass_download = details['do_mass_download'] master_suites = details['master_suites'] - if skip_timeseries == None: + if skip_timeseries is None: pass else: do_analysis_timeseries = not skip_timeseries From ecd5ade6b4a0bbe6c882f06183985ae0d8d90b22 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Fri, 11 Aug 2023 13:52:43 +0100 Subject: [PATCH 04/22] cleanup and and pin numpy --- environment.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/environment.yml b/environment.yml index bd591109..fd42d47c 100644 --- a/environment.yml +++ b/environment.yml @@ -10,15 +10,15 @@ dependencies: - matplotlib - nctoolkit >=0.8.7 # use linux64 build - netcdf4 - - numpy !=1.24.3 + - numpy >1.24.3, <1.25 # basemap=1.3.7 needs <1.25 - pip !=21.3 - python >=3.9 - pyyaml - - scipy + - scipy >=1.6 - scikit-learn # Python packages - dependencies for testing - flake8 - - iris + - iris >=3.4.0 - pytest >=3.9,!=6.0.0rc1,!=6.0.0 - pytest-cov >=2.10.1 - pytest-env From 2e871e2032e06d2e71459adae3739f02fc2ece2e Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Fri, 11 Aug 2023 13:52:57 +0100 Subject: [PATCH 05/22] cleanup and and pin numpy --- setup.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/setup.py b/setup.py index 859c7f46..015512b2 100755 --- a/setup.py +++ b/setup.py @@ -29,20 +29,20 @@ 'matplotlib', 'nctoolkit>=0.8.7', # use linux64 build 'netcdf4', - 'numpy!=1.24.3', + 'numpy>1.24.3,<1.25', # basemap 1.3.7 needs <1.25 'pip!=21.3', 'pyyaml', 'scikit-learn', - 'scipy', + 'scipy>=1.6', ], # Test dependencies # Execute 'python setup.py test' to run tests 'test': [ 'flake8', + 'scitools-iris>=3.4.0', 'pytest>=3.9,!=6.0.0rc1,!=6.0.0', 'pytest-cov>=2.10.1', 'pytest-env', - 'pytest-flake8>=1.0.6', 'pytest-html!=2.1.0', 'pytest-metadata>=1.5.1', 'pytest-mypy', @@ -58,7 +58,7 @@ 'isort', 'pre-commit', 'prospector[with_pyroma,with_mypy]>=1.9.0', - 'sphinx>2', + 'sphinx>=5', 'sphinx_rtd_theme', 'vprof', 'yamllint', From e7274bfcac7e6f377f27de0611345039e927cfec Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Fri, 11 Aug 2023 13:54:03 +0100 Subject: [PATCH 06/22] run a full GA --- .github/workflows/run-tests.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 466a8d4b..e034be48 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -5,6 +5,7 @@ on: push: branches: - main + - cleanup_env_numpy_from_condaforge schedule: - cron: '0 0 * * *' # nightly From a205300a0a9135853835dac15c3accd815c24a12 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Fri, 11 Aug 2023 13:59:18 +0100 Subject: [PATCH 07/22] UNrun a full GA --- .github/workflows/run-tests.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index e034be48..466a8d4b 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -5,7 +5,6 @@ on: push: branches: - main - - cleanup_env_numpy_from_condaforge schedule: - cron: '0 0 * * *' # nightly From 55666bac24cecb605d20d4951cd179a45f62d041 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 19 Oct 2023 14:50:06 +0100 Subject: [PATCH 08/22] Added permission check before editing files/copying --- bgcval2/download_from_mass.py | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/bgcval2/download_from_mass.py b/bgcval2/download_from_mass.py index 461e24c8..463c39b5 100755 --- a/bgcval2/download_from_mass.py +++ b/bgcval2/download_from_mass.py @@ -37,6 +37,7 @@ from socket import gethostname import shutil import os +from pwd import getpwuid import stat from glob import glob from re import findall @@ -409,7 +410,16 @@ def download_from_mass( outputFold = folder([paths.ModelFolder_pref, jobID,] ) # make this folder group writeable. st = os.stat(outputFold) - os.chmod(outputFold, st.st_mode | stat.S_IWGRP) + + # Case one: + # I just created this folder and I own it: + i_can_write_this = os.access(outputFold, os.W_OK) + if i_can_write_this: + os.chmod(outputFold, st.st_mode | stat.S_IWGRP) + else: + uname = getpwuid(os.stat(outputFold).st_uid).pw_name + print('WARNING: someone else (', uname, ') owns this directory, so you may not be able to download files.') + print('WARNING: Ask them politely to run "chmod g+w ',outputFold,'"') deleteBadLinksAndZeroSize(outputFold, jobID) @@ -483,8 +493,19 @@ def download_from_mass( if auto_download: shared_file_path = os.path.join(paths.shared_mass_scripts, os.path.basename(download_script_path)) print('writing file in shared path', shared_file_path) - shutil.copy(download_script_path, shared_file_path) + # Case one: + # I just created this folder and I own it: + i_can_write_this = os.access(shared_file_path, os.W_OK) + if i_can_write_this: + os.chmod(download_script_path, st.st_mode | stat.S_IWGRP) + shutil.copy(download_script_path, shared_file_path) + os.chmod(shared_file_path, st.st_mode | stat.S_IWGRP) + else: + uname = getpwuid(os.stat(shared_file_path).st_uid).pw_name + print('WARNING: someone else (', shared_file_path, ') owns this directory, so you may not be able to download files.') + print('WARNING: Ask them politely to run "chmod g+w ', shared_file_path,'"') + fixFilePaths(outputFold, jobID, debug=False,) deleteBadLinksAndZeroSize(outputFold, jobID, debug=False,) @@ -598,12 +619,17 @@ def deleteBadLinksAndZeroSize(outputFold, jobID, debug=True): bashCommand1 = "find " + outputFold + "/. -size 0 -print -delete" bashCommand2 = "find -L " + outputFold + "/. -type l -delete -print" - if debug: print("deleteBadLinksAndZeroSize:\t", bashCommand1) + bashCommand1 = bashCommand1.replace('//', '/') + bashCommand2 = bashCommand2.replace('//', '/') + + #f debug: + print("deleteBadLinksAndZeroSize:\t", bashCommand1) process1 = subprocess.Popen(bashCommand1.split(), stdout=subprocess.PIPE) output1 = process1.communicate()[0] - if debug: print("deleteBadLinksAndZeroSize:\t", bashCommand2) + #f debug: + print("deleteBadLinksAndZeroSize:\t", bashCommand2) process2 = subprocess.Popen(bashCommand2.split(), stdout=subprocess.PIPE) output2 = process2.communicate()[0] From bb1b36efd3ce2fe4034a301cc28f6a8860bb4222 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 19 Oct 2023 14:52:33 +0100 Subject: [PATCH 09/22] minor comment edit --- bgcval2/download_from_mass.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/bgcval2/download_from_mass.py b/bgcval2/download_from_mass.py index 463c39b5..351fc657 100755 --- a/bgcval2/download_from_mass.py +++ b/bgcval2/download_from_mass.py @@ -412,7 +412,7 @@ def download_from_mass( st = os.stat(outputFold) # Case one: - # I just created this folder and I own it: + # I created this folder and I own it: i_can_write_this = os.access(outputFold, os.W_OK) if i_can_write_this: os.chmod(outputFold, st.st_mode | stat.S_IWGRP) @@ -494,8 +494,7 @@ def download_from_mass( shared_file_path = os.path.join(paths.shared_mass_scripts, os.path.basename(download_script_path)) print('writing file in shared path', shared_file_path) - # Case one: - # I just created this folder and I own it: + # I created this file and I own it: i_can_write_this = os.access(shared_file_path, os.W_OK) if i_can_write_this: os.chmod(download_script_path, st.st_mode | stat.S_IWGRP) From ba7738ca1c5ecbd8a2ec0cecb90aac671d4c5e78 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 19 Oct 2023 15:17:27 +0100 Subject: [PATCH 10/22] improved comments --- bgcval2/download_from_mass.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/bgcval2/download_from_mass.py b/bgcval2/download_from_mass.py index 351fc657..756ccf55 100755 --- a/bgcval2/download_from_mass.py +++ b/bgcval2/download_from_mass.py @@ -408,16 +408,18 @@ def download_from_mass( paths = get_paths(config_user) outputFold = folder([paths.ModelFolder_pref, jobID,] ) - # make this folder group writeable. st = os.stat(outputFold) - - # Case one: - # I created this folder and I own it: + + # Check permissions on the output folder i_can_write_this = os.access(outputFold, os.W_OK) if i_can_write_this: + # I created this folder and I own it. + # make this folder group writeable. os.chmod(outputFold, st.st_mode | stat.S_IWGRP) else: - uname = getpwuid(os.stat(outputFold).st_uid).pw_name + # Someone else created it and they own it, + # so I can't change permissions. + uname = getpwuid(st.st_uid).pw_name print('WARNING: someone else (', uname, ') owns this directory, so you may not be able to download files.') print('WARNING: Ask them politely to run "chmod g+w ',outputFold,'"') @@ -494,13 +496,20 @@ def download_from_mass( shared_file_path = os.path.join(paths.shared_mass_scripts, os.path.basename(download_script_path)) print('writing file in shared path', shared_file_path) - # I created this file and I own it: + # check destination permissions: i_can_write_this = os.access(shared_file_path, os.W_OK) if i_can_write_this: + # I created this file and I own it: + # make local copy group writable: os.chmod(download_script_path, st.st_mode | stat.S_IWGRP) + + # copy local to shared folder: shutil.copy(download_script_path, shared_file_path) + + # make shared copy group writable: (possibly overkill) os.chmod(shared_file_path, st.st_mode | stat.S_IWGRP) else: + # I don't have permission to edit this file. Someone else does: uname = getpwuid(os.stat(shared_file_path).st_uid).pw_name print('WARNING: someone else (', shared_file_path, ') owns this directory, so you may not be able to download files.') print('WARNING: Ask them politely to run "chmod g+w ', shared_file_path,'"') @@ -621,15 +630,11 @@ def deleteBadLinksAndZeroSize(outputFold, jobID, debug=True): bashCommand1 = bashCommand1.replace('//', '/') bashCommand2 = bashCommand2.replace('//', '/') - #f debug: print("deleteBadLinksAndZeroSize:\t", bashCommand1) - process1 = subprocess.Popen(bashCommand1.split(), stdout=subprocess.PIPE) output1 = process1.communicate()[0] - #f debug: print("deleteBadLinksAndZeroSize:\t", bashCommand2) - process2 = subprocess.Popen(bashCommand2.split(), stdout=subprocess.PIPE) output2 = process2.communicate()[0] From 17c01ddf87b95ea06cc18a91e323daec86b08ff2 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Mon, 23 Oct 2023 15:12:52 +0100 Subject: [PATCH 11/22] add unit test dir --- .../test_download_from_mass_permissions.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 tests/unit/test_download_from_mass_permissions.py diff --git a/tests/unit/test_download_from_mass_permissions.py b/tests/unit/test_download_from_mass_permissions.py new file mode 100644 index 00000000..cdefa6a8 --- /dev/null +++ b/tests/unit/test_download_from_mass_permissions.py @@ -0,0 +1,32 @@ +import os +import shutil +import stat + +from bgcval2.download_from_mass import download_from_mass as dfm + + +def test_allowed_user(): + dfm(jobID="u-xxx", + doMoo=False, + auto_download=False, + config_user="defaults" + ) + user_home = os.path.expanduser('~') + output_folder = "bgcval2/local_test/BGC_data/u-xxx/" + run_dir = os.path.join(user_home, output_folder) + assert os.stat(run_dir).st_mode == 16893 + + +def test_disallowed_user(): + user_home = os.path.expanduser('~') + output_folder = "bgcval2/local_test/BGC_data/u-xxx/" + run_dir = os.path.join(user_home, output_folder) + st = os.stat(run_dir) + os.chmod(run_dir, False) + dfm(jobID="u-xxx", + doMoo=False, + auto_download=False, + config_user="defaults" + ) + assert os.stat(run_dir).st_mode == 16384 + shutil.rmtree(run_dir, ignore_errors=True) From 06f4336202e381d6d698e2a4c654a07285814020 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Mon, 23 Oct 2023 15:13:17 +0100 Subject: [PATCH 12/22] remove whitespace --- bgcval2/download_from_mass.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgcval2/download_from_mass.py b/bgcval2/download_from_mass.py index 756ccf55..a42c03da 100755 --- a/bgcval2/download_from_mass.py +++ b/bgcval2/download_from_mass.py @@ -409,7 +409,7 @@ def download_from_mass( paths = get_paths(config_user) outputFold = folder([paths.ModelFolder_pref, jobID,] ) st = os.stat(outputFold) - + # Check permissions on the output folder i_can_write_this = os.access(outputFold, os.W_OK) if i_can_write_this: From 9d032464a2de217fce5662d8a17d4c2c913036d6 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Mon, 23 Oct 2023 15:20:34 +0100 Subject: [PATCH 13/22] tweak test --- tests/unit/test_download_from_mass_permissions.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_download_from_mass_permissions.py b/tests/unit/test_download_from_mass_permissions.py index cdefa6a8..1471121e 100644 --- a/tests/unit/test_download_from_mass_permissions.py +++ b/tests/unit/test_download_from_mass_permissions.py @@ -19,11 +19,13 @@ def test_allowed_user(): def test_disallowed_user(): user_home = os.path.expanduser('~') - output_folder = "bgcval2/local_test/BGC_data/u-xxx/" + output_folder = "bgcval2/local_test/BGC_data/u-yyy/" run_dir = os.path.join(user_home, output_folder) + if not os.path.exists(run_dir): + os.makedirs(run_dir) st = os.stat(run_dir) os.chmod(run_dir, False) - dfm(jobID="u-xxx", + dfm(jobID="u-yyy", doMoo=False, auto_download=False, config_user="defaults" From 098ad28c3dd47f25494e0b69b3064b819a6ceef0 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Mon, 23 Oct 2023 16:09:08 +0100 Subject: [PATCH 14/22] fix test --- tests/unit/test_download_from_mass_permissions.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_download_from_mass_permissions.py b/tests/unit/test_download_from_mass_permissions.py index 1471121e..719a78fc 100644 --- a/tests/unit/test_download_from_mass_permissions.py +++ b/tests/unit/test_download_from_mass_permissions.py @@ -6,14 +6,16 @@ def test_allowed_user(): + user_home = os.path.expanduser('~') + output_folder = "bgcval2/local_test/BGC_data/u-xxx/" + run_dir = os.path.join(user_home, output_folder) + if not os.path.exists(run_dir): + os.makedirs(run_dir) dfm(jobID="u-xxx", doMoo=False, auto_download=False, config_user="defaults" ) - user_home = os.path.expanduser('~') - output_folder = "bgcval2/local_test/BGC_data/u-xxx/" - run_dir = os.path.join(user_home, output_folder) assert os.stat(run_dir).st_mode == 16893 From 4300bbd54a40a8f4a7e52884bc04987a97b1fc01 Mon Sep 17 00:00:00 2001 From: Valeriu Predoi Date: Mon, 23 Oct 2023 16:27:51 +0100 Subject: [PATCH 15/22] allow for 16877 --- tests/unit/test_download_from_mass_permissions.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_download_from_mass_permissions.py b/tests/unit/test_download_from_mass_permissions.py index 719a78fc..6c56c373 100644 --- a/tests/unit/test_download_from_mass_permissions.py +++ b/tests/unit/test_download_from_mass_permissions.py @@ -16,7 +16,11 @@ def test_allowed_user(): auto_download=False, config_user="defaults" ) - assert os.stat(run_dir).st_mode == 16893 + try: + assert os.stat(run_dir).st_mode == 16893 + # when stat mode is 16877 (equivalent to chmod 877) + except AssertionError: + assert os.stat(run_dir).st_mode == 16877 def test_disallowed_user(): From 23c826a4fb1416f8e3cb61022b240243a4377844 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 2 Nov 2023 10:40:21 +0000 Subject: [PATCH 16/22] added fixes into the correct branch this time. --- bgcval2/download_from_mass.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/bgcval2/download_from_mass.py b/bgcval2/download_from_mass.py index a42c03da..e2a3cd68 100755 --- a/bgcval2/download_from_mass.py +++ b/bgcval2/download_from_mass.py @@ -412,15 +412,22 @@ def download_from_mass( # Check permissions on the output folder i_can_write_this = os.access(outputFold, os.W_OK) - if i_can_write_this: + owner = getpwuid(st.st_uid).pw_name + my_username = os.environ.get('USERNAME') + + i_own_this = my_username == owner + #group_can_write_this = os.access(outputFold, os.W_OK) + if i_can_write_this and i_own_this: # I created this folder and I own it. - # make this folder group writeable. + # make this folder group writeable.: os.chmod(outputFold, st.st_mode | stat.S_IWGRP) + elif i_can_write_this and not i_own_this: + pass else: # Someone else created it and they own it, # so I can't change permissions. - uname = getpwuid(st.st_uid).pw_name - print('WARNING: someone else (', uname, ') owns this directory, so you may not be able to download files.') + owner = getpwuid(st.st_uid).pw_name + print('WARNING: someone else (', owner, ') owns this directory, so you may not be able to download files.') print('WARNING: Ask them politely to run "chmod g+w ',outputFold,'"') deleteBadLinksAndZeroSize(outputFold, jobID) @@ -503,11 +510,12 @@ def download_from_mass( # make local copy group writable: os.chmod(download_script_path, st.st_mode | stat.S_IWGRP) - # copy local to shared folder: - shutil.copy(download_script_path, shared_file_path) + # copy local to shared folder: + if not os.path.exists(shared_file_path): + shutil.copy(download_script_path, shared_file_path) - # make shared copy group writable: (possibly overkill) - os.chmod(shared_file_path, st.st_mode | stat.S_IWGRP) + # make shared copy group writable: (possibly overkill) + os.chmod(shared_file_path, st.st_mode | stat.S_IWGRP) else: # I don't have permission to edit this file. Someone else does: uname = getpwuid(os.stat(shared_file_path).st_uid).pw_name From 34b44ba32ff3b85a8c9c155e76fd326e4c655172 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 2 Nov 2023 14:24:17 +0000 Subject: [PATCH 17/22] fixed missing file case --- bgcval2/download_from_mass.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bgcval2/download_from_mass.py b/bgcval2/download_from_mass.py index e2a3cd68..1270c1de 100755 --- a/bgcval2/download_from_mass.py +++ b/bgcval2/download_from_mass.py @@ -503,8 +503,14 @@ def download_from_mass( shared_file_path = os.path.join(paths.shared_mass_scripts, os.path.basename(download_script_path)) print('writing file in shared path', shared_file_path) - # check destination permissions: - i_can_write_this = os.access(shared_file_path, os.W_OK) + # check destination existance and permissions + if os.path.exists(shared_file_path): + # check destination permissions + i_can_write_this = os.access(shared_file_path, os.W_OK) + else: + # File doesn't exist! + i_can_write_this = True + if i_can_write_this: # I created this file and I own it: # make local copy group writable: From 841f07369c0cf34a875d9cfa8f73f3598ad3d89c Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 2 Nov 2023 15:23:09 +0000 Subject: [PATCH 18/22] added capability to choose moving average and labels in the yml files --- bgcval2/analysis_compare.py | 19 ++++++++-- bgcval2/analysis_timeseries.py | 5 +++ bgcval2/timeseries/timeseriesPlots.py | 53 ++++++++++++++------------- key_files/amoc_26n.yml | 1 + 4 files changed, 49 insertions(+), 29 deletions(-) diff --git a/bgcval2/analysis_compare.py b/bgcval2/analysis_compare.py index e1c3be68..f1d6b833 100755 --- a/bgcval2/analysis_compare.py +++ b/bgcval2/analysis_compare.py @@ -189,6 +189,7 @@ def timeseries_compare(jobs, jobDescriptions={}, lineThicknesses=defaultdict(lambda: 1), linestyles=defaultdict(lambda: '-'), + labels = {}, ensembles={}, config_user=None, ): @@ -382,6 +383,7 @@ def timeseries_compare(jobs, regions = av[name]['regions'] layers = av[name]['layers'] metrics = av[name]['metrics'] + smoothings = av[name]['smoothings'] for region, layer, metric in itertools.product(regions, layers, metrics): timesD = {} arrD = {} @@ -405,20 +407,22 @@ def timeseries_compare(jobs, units = av[name]['modeldetails']['units'] ts = 'Together' - for ls in ['DataOnly', ]: + for smoothing in smoothings: + #or ls in ['DataOnly', ]: tsp.multitimeseries( timesD, # model times (in floats) arrD, # model time series data=-999, # in situ data distribution title=title, filename=bvt.folder(imageFolder) + - '_'.join([name, region, layer, ts, ls + '.png']), + '_'.join([name, region, layer, ts, smoothing + '.png']), units=units, plotStyle=ts, - lineStyle=ls, + smoothing=smoothing, colours=colours, thicknesses=lineThicknesses, linestyles=linestyles, + labels=labels, ) # Generate a list of comparison images: @@ -515,7 +519,8 @@ def load_comparison_yml(master_compare_yml_fn): descriptions = {} shifttimes = {} # number of years to shift time axis. timeranges = {} - + labels = {} + for jobID, job_dict in details['jobs'].items(): if job_dict.get('colour', False): colours[jobID] = job_dict['colour'] @@ -530,6 +535,8 @@ def load_comparison_yml(master_compare_yml_fn): timeranges[jobID] = job_dict.get('timerange', None) suites[jobID] = job_dict.get('suite', default_suite) auto_download_dict[jobID] = job_dict.get('auto_download', auto_download_dict[jobID]) + labels[jobID] = job_dict.get('label', jobID) + details['colours'] = colours details['descriptions'] = descriptions @@ -537,6 +544,7 @@ def load_comparison_yml(master_compare_yml_fn): details['linestyles'] = linestyles details['shifttimes'] = shifttimes details['timeranges'] = timeranges + details['labels'] = labels details['suites'] = suites details['auto_download'] = auto_download_dict return details @@ -567,6 +575,7 @@ def load_yml_and_run(compare_yml, config_user, skip_timeseries): descriptions = details['descriptions'] shifttimes = details['shifttimes'] timeranges = details['timeranges'] + labels = details['labels'] suites = details['suites'] auto_download = details['auto_download'] strictFileCheck = details.get('strictFileCheck', True) @@ -578,6 +587,7 @@ def load_yml_and_run(compare_yml, config_user, skip_timeseries): print(jobID, 'colour:',colours[jobID]) print(jobID, 'line thickness & style:',thicknesses[jobID], linestyles[jobID]) print(jobID, 'Shift time by', shifttimes[jobID]) + print(jobID, 'Label: ', labels[jobID]) print(jobID, 'Time range (None means all):', timeranges.get(jobID, None)) print(jobID, 'suite:', suites[jobID]) print(jobID, 'auto_download', auto_download[jobID]) @@ -620,6 +630,7 @@ def load_yml_and_run(compare_yml, config_user, skip_timeseries): analysisname=analysis_name, lineThicknesses=thicknesses, linestyles=linestyles, + labels=labels, config_user=config_user, ) diff --git a/bgcval2/analysis_timeseries.py b/bgcval2/analysis_timeseries.py index d063dfa8..e6aa75d0 100755 --- a/bgcval2/analysis_timeseries.py +++ b/bgcval2/analysis_timeseries.py @@ -297,6 +297,11 @@ def load_key_file(key, paths, jobID, strictFileCheck=True): output_dict['metrics'] = key_dict.get('metrics', metricList) output_dict['metrics'] = parse_list_from_string(output_dict['metrics']) + # Smoothings: + default_smoothings = ['DataOnly', ] + output_dict['smoothings'] = key_dict.get('smoothings', default_smoothings) + output_dict['smoothings'] = parse_list_from_string(output_dict['smoothings']) + # Load Grid: gridFile = key_dict.get('gridFile', paths.orcaGridfn) output_dict['gridFile'] = list_input_files(gridFile, key_dict, paths)[0] diff --git a/bgcval2/timeseries/timeseriesPlots.py b/bgcval2/timeseries/timeseriesPlots.py index 543849f4..aecaaca3 100644 --- a/bgcval2/timeseries/timeseriesPlots.py +++ b/bgcval2/timeseries/timeseriesPlots.py @@ -702,9 +702,10 @@ def multitimeseries( 'lime', ], plotStyle='Together', - lineStyle='', + smoothing='', thicknesses=defaultdict(lambda: 1), linestyles=defaultdict(lambda: '-'), + labels={}, ): if 0 in [len(timesD), len(list(timesD.keys()))]: return @@ -783,7 +784,8 @@ def multitimeseries( if np.min(arr) < ylims[0]: ylims[0] = np.min(arr) if np.max(arr) > ylims[1]: ylims[1] = np.max(arr) - if lineStyle.lower() in ['spline', 'all']: + label = labels.get(jobID, jobID) + if smoothing.lower() in ['spline', 'all']: tnew = np.linspace(times[0], times[-1], 60) arr_smooth = interpolate.spline(times, arr, tnew) pyplot.plot( @@ -792,10 +794,10 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID + ' spline', + label=label + ' spline', ) - if lineStyle.lower() in ['movingaverage', 'both', 'all']: + if smoothing.lower() in ['movingaverage', 'both', 'all']: if len(times) > 100.: window = 30 elif len(times) > 30.: window = 15 elif len(times) > 10.: window = 4 @@ -810,10 +812,10 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID, + label=label, ) - if lineStyle.lower() in [ + if smoothing.lower() in [ 'movingaverage5', ]: window = 5 @@ -827,11 +829,11 @@ def multitimeseries( arr_new, c=colours[jobID], ls=linestyles[jobID], - label=jobID, + label=label, lw=thicknesses[jobID], ) - if lineStyle.lower() in [ + if smoothing.lower() in [ 'movingav1year', ]: arr_new = movingaverage_DT(arr, @@ -844,10 +846,10 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID, + label=label, ) - if lineStyle.lower() in [ - 'movingav5years', + if smoothing.lower() in [ + 'movingav5years', 'both5' ]: arr_new = movingaverage_DT(arr, times, @@ -859,11 +861,11 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID, + label=label, ) - if lineStyle.lower() in [ - 'movingav30years', + if smoothing.lower() in [ + 'movingav30years', 'both30' ]: pyplot.plot(times, arr, @@ -881,11 +883,11 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID, + label=label, ) - if lineStyle.lower() in [ - 'movingav100years', + if smoothing.lower() in [ + 'movingav100years', 'both100', ]: pyplot.plot(times, arr, @@ -903,10 +905,10 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID, + label=label, ) - if lineStyle.lower() in [ + if smoothing.lower() in [ 'movingaverage12', ]: window = 12 @@ -921,10 +923,10 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=2., - label=jobID, + label=label, ) - if lineStyle.lower() in [ + if smoothing.lower() in [ 'movingaverage60', ]: window = 60 @@ -939,13 +941,14 @@ def multitimeseries( c=colours[jobID], ls=linestyles[jobID], lw=2., - label=jobID, + label=label, ) - if lineStyle.lower() in [ + if smoothing.lower() in [ '', 'both', 'all', + 'both5', 'both30', 'both100', ]: pyplot.plot(times, arr, @@ -953,14 +956,14 @@ def multitimeseries( ls=linestyles[jobID], lw=0.25) - if lineStyle.lower() in ['dataonly']: + if smoothing.lower() in ['dataonly']: pyplot.plot( times, arr, c=colours[jobID], ls=linestyles[jobID], lw=thicknesses[jobID], - label=jobID, + label=label, ) if type(data) == type(10.): diff --git a/key_files/amoc_26n.yml b/key_files/amoc_26n.yml index 5b77b064..2d99d7a3 100644 --- a/key_files/amoc_26n.yml +++ b/key_files/amoc_26n.yml @@ -13,3 +13,4 @@ model_convert: altmaskfile: $PATHS_BGCVAL2/bgcval2/data/basinlandmask_eORCA1.nc layers: layerless regions: regionless +smoothings: DataOnly both5 both30 movingav30years From f54cb6c8411b4bbf7879dd20306b16e1751287da Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Thu, 2 Nov 2023 15:38:26 +0000 Subject: [PATCH 19/22] added more smoothing options --- bgcval2/timeseries/timeseriesPlots.py | 54 +++++++++++++++++++++++++++ key_files/amoc_26n.yml | 2 +- key_files/drakepassagetransport.yml | 2 + 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/bgcval2/timeseries/timeseriesPlots.py b/bgcval2/timeseries/timeseriesPlots.py index aecaaca3..217b8d21 100644 --- a/bgcval2/timeseries/timeseriesPlots.py +++ b/bgcval2/timeseries/timeseriesPlots.py @@ -833,6 +833,60 @@ def multitimeseries( lw=thicknesses[jobID], ) + if smoothing.lower() == '5and30': + arr_new5 = movingaverage_DT(arr, + times, + window_len=5., + window_units='years') + + arr_new30 = movingaverage_DT(arr, + times, + window_len=30., + window_units='years') + + pyplot.plot( + times, + arr_new5, + c=colours[jobID], + ls=linestyles[jobID], + lw=thicknesses[jobID]/2., + ) + pyplot.plot( + times, + arr_new30, + c=colours[jobID], + ls=linestyles[jobID], + lw=thicknesses[jobID], + label=label, + ) + if smoothing.lower() == '30and100': + arr_new5 = movingaverage_DT(arr, + times, + window_len=30., + window_units='years') + + arr_new30 = movingaverage_DT(arr, + times, + window_len=100., + window_units='years') + + pyplot.plot( + times, + arr_new5, + c=colours[jobID], + ls=linestyles[jobID], + lw=thicknesses[jobID]/2., + ) + pyplot.plot( + times, + arr_new30, + c=colours[jobID], + ls=linestyles[jobID], + lw=thicknesses[jobID], + label=label, + ) + + if smoothing.lower() in [ 'movingav1year', ]: diff --git a/key_files/amoc_26n.yml b/key_files/amoc_26n.yml index 2d99d7a3..9577ac97 100644 --- a/key_files/amoc_26n.yml +++ b/key_files/amoc_26n.yml @@ -13,4 +13,4 @@ model_convert: altmaskfile: $PATHS_BGCVAL2/bgcval2/data/basinlandmask_eORCA1.nc layers: layerless regions: regionless -smoothings: DataOnly both5 both30 movingav30years +smoothings: DataOnly both5 both30 movingav30years 5and30 30and100 diff --git a/key_files/drakepassagetransport.yml b/key_files/drakepassagetransport.yml index 776b572e..124f8b33 100644 --- a/key_files/drakepassagetransport.yml +++ b/key_files/drakepassagetransport.yml @@ -11,3 +11,5 @@ model_convert: areafile: $PATHS_GRIDFILE layers: layerless regions: regionless +smoothings: DataOnly 5and30 30and100 + From c28346d0e5809be9697941f197a97bf1fcbe6dbe Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Mon, 6 Nov 2023 08:11:55 +0000 Subject: [PATCH 20/22] Update bgcval2/timeseries/timeseriesPlots.py Co-authored-by: Valeriu Predoi --- bgcval2/timeseries/timeseriesPlots.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgcval2/timeseries/timeseriesPlots.py b/bgcval2/timeseries/timeseriesPlots.py index 217b8d21..c34115a2 100644 --- a/bgcval2/timeseries/timeseriesPlots.py +++ b/bgcval2/timeseries/timeseriesPlots.py @@ -702,7 +702,7 @@ def multitimeseries( 'lime', ], plotStyle='Together', - smoothing='', + smoothing='', thicknesses=defaultdict(lambda: 1), linestyles=defaultdict(lambda: '-'), labels={}, From e9dd4df4fc925d2067239ca8bd235ff2826d2878 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Mon, 6 Nov 2023 11:16:41 +0000 Subject: [PATCH 21/22] Added documentation --- README.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/README.md b/README.md index 1a05ee71..5d8c6c7b 100644 --- a/README.md +++ b/README.md @@ -229,6 +229,7 @@ strictFileCheck: jobs: : description: + label: 'Job info' colour: red thickness: 0.7 linestyle: '-' @@ -271,6 +272,10 @@ These values are: - The options are: - `description`: - A description of job, which helps people differentiate the jobs in the final report. + - `label`: + - A short description of the job which will appear in the plot legend. + - If you make it too long, it won't fit on the plot. + - Optional: Default behaviour is the jobID. - `colour`: - The colour of this jobs line in the report plots. - Default colour is a randomly generated hex colour. @@ -447,6 +452,8 @@ units: Units dimensions: 1,2 or 3 # The number of dimensions after the calculations are performed layers : Surface regions : Global ignoreInlandSeas SouthernOcean ArcticOcean Equator10 Remainder NorthernSubpolarAtlantic NorthernSubpolarPacific +smoothings : DataOnly both5 both30 movingav30years 5and30 30and100 + #paths: modelFiles : $BASEDIR_MODEL/$JOBID/nemo_$JOBIDo_1y_*_grid-T.nc @@ -500,6 +507,26 @@ basic functions such as multiply by or add to, or `noChange`, which can all be called without providing the `path`, and which may have their own key word arguments. +Some of the other options for each analysis include: + - `layers`: + - The z axis layers that you want to use. + - Typically include things like `Surface`, `500m`, `1000m` etc. + - Note that this is a pre-curated list, so you can't use it to select a specific depth (like `327m` or something) + - `regions`: + - A list of regions to investigate. + - This is a pre-curated list, and they are defined in so you can't. + - These regions are defined in the file `bgcval2/bgcvaltools/makeMask.py`. + - `smoothings`: + - This is the smoothing function (if any) to apply to the data before plotting it. + - The smoothing it added before plotting, but after saving the shelve file, so it doesn't impact the data. + - No smoothing is `DataOnly`, which is also the default behaviour. + - If several smoothing options are added here, bgcval2 will generate a comparison plot for each one. + - Other modes exist: + - `movingav30years`: A 30 year moving average + - `both5`: Both no smoothing and a 5 year moving average + - `both30`: Both no smoothing and a 30 year moving average + - `5and30`: a 5 year moving average and a 30 year moving average. + Clearing the Cache ------------------ From 39661765298f976e04dc549e872e97567d6db9c4 Mon Sep 17 00:00:00 2001 From: Lee de Mora Date: Mon, 6 Nov 2023 11:25:12 +0000 Subject: [PATCH 22/22] Update README.md --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 5d8c6c7b..3c9e8aee 100644 --- a/README.md +++ b/README.md @@ -521,6 +521,7 @@ Some of the other options for each analysis include: - The smoothing it added before plotting, but after saving the shelve file, so it doesn't impact the data. - No smoothing is `DataOnly`, which is also the default behaviour. - If several smoothing options are added here, bgcval2 will generate a comparison plot for each one. + - The smoothings are defined and performed in `bgcval2/timeseries/timeseriesPlots.py` - Other modes exist: - `movingav30years`: A 30 year moving average - `both5`: Both no smoothing and a 5 year moving average