Bias correction #292

richardarsenault · 2020-07-22T18:35:21Z

Adds interacton with Xclim for bias correction with a simple test suite.

Update calls and dependencies to support newest xclim version

Else `make develop` fail to install the newer `gdal` using `pip` (compile error below). `gdal` was updated in requirements.txt in this commit 0c57bb4. ``` gcc -pthread -B /home/lvu/.conda/envs/raven/compiler_compat -Wl,--sysroot=/ -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I../../port -I../../gcore -I../../alg -I../../ogr/ -I../../og r/ogrsf_frmts -I../../gnm -I../../apps -I/home/lvu/.conda/envs/raven/include/python3.7m -I/home/lvu/.conda/envs/raven/lib/python3.7/site-packages/numpy/core/include -I/home/lvu/.conda/envs/raven/include -c exten sions/gdal_wrap.cpp -o build/temp.linux-x86_64-3.7/extensions/gdal_wrap.o -I/home/lvu/.conda/envs/raven/include cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for C/ObjC but not for C++ extensions/gdal_wrap.cpp: In function ‘OSRSpatialReferenceShadow* GDALDatasetShadow_GetSpatialRef(GDALDatasetShadow*)’: extensions/gdal_wrap.cpp:4672:32: error: ‘GDALGetSpatialRef’ was not declared in this scope OGRSpatialReferenceH ref = GDALGetSpatialRef(self); ^~~~~~~~~~~~~~~~~ ```

environment.yml: sync gdal version with requirements.txt Else `make develop` fail to install the newer `gdal` using `pip` (compile error below). `gdal` was updated in `requirements.txt` in this commit 0c57bb4. ``` gcc -pthread -B /home/lvu/.conda/envs/raven/compiler_compat -Wl,--sysroot=/ -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I../../port -I../../gcore -I../../alg -I../../ogr/ -I../../og r/ogrsf_frmts -I../../gnm -I../../apps -I/home/lvu/.conda/envs/raven/include/python3.7m -I/home/lvu/.conda/envs/raven/lib/python3.7/site-packages/numpy/core/include -I/home/lvu/.conda/envs/raven/include -c exten sions/gdal_wrap.cpp -o build/temp.linux-x86_64-3.7/extensions/gdal_wrap.o -I/home/lvu/.conda/envs/raven/include cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for C/ObjC but not for C++ extensions/gdal_wrap.cpp: In function ‘OSRSpatialReferenceShadow* GDALDatasetShadow_GetSpatialRef(GDALDatasetShadow*)’: extensions/gdal_wrap.cpp:4672:32: error: ‘GDALGetSpatialRef’ was not declared in this scope OGRSpatialReferenceH ref = GDALGetSpatialRef(self); ^~~~~~~~~~~~~~~~~ ```

"birdhouse/1-Datasets" was the first version and won't receive auto NCML deploy update anymore. Will be removed eventually to avoid future confusions.

tests + notebooks: use new official NCML link "birdhouse/1-Datasets" was the first version and won't receive auto NCML deploy update anymore. Will be removed eventually to avoid future confusions.

richardarsenault · 2020-07-22T18:35:53Z

@tvlu can you check for the conflicts please?

tlvu

@Zeitsperre Few questions for you since I have not been following this branch.

tlvu · 2020-07-22T19:15:40Z

.travis.yml

@@ -56,6 +52,7 @@ jobs:
 branches:
  only:
    - master
+    - bias_correction


Why do we want to limit Travis-CI to only master branch, then we have to add each dev branch manually, like this bias_correction branch? @Zeitsperre (b5a3566)

This can be removed. It was placed in order to trigger the bias_correction branch that PRs were merging to in a few instances. The reason why I set only to master is that otherwise we tend to get two duplicate sets of builds; one that is the pushes set against master and another that is the pushes set against the last version of master it stemmed from.

Since we only care about the current state of master, as all pushes must integrate these changes, there's nothing to gain except more redundant build checks.

I think the redundant build checks on master branch will happen only once, when the PR is merged to master.

Whereas instructions to build only the master branch will not test any commits on any devel branches and force all devel branches to hardcode the devel branch name like we just saw.

I think having the redundant build on master is a less bad of the 2 problems. Wonder what's your opinion about removing this "build only master branch" instruction for all birds?

tlvu · 2020-07-22T19:18:18Z

.travis.yml

-#  - raven start --daemon --bind-host 0.0.0.0 --port 5000
-  - bash -c "source $HOME/miniconda3/bin/activate raven && make start"
+    #  - raven start --daemon --bind-host 0.0.0.0 --port 5000
+    - bash -c "source $HOME/miniconda3/bin/activate raven && pip install git+https://github.com/pydata/xarray.git@master"


Is this install from xarray.git@master still necessary with xarray 0.16 from master branch? @Zeitsperre (a125ff4)

With xarray at 0.16, the sdba functions that Pascal was working on are integrated, so no. This workaround can blissfully be thrown in the trash.

Running pep8 code style checks ... tests/test_bias_correction.py:8:1: W293 blank line contains whitespace tests/test_bias_correction.py:9:17: E225 missing whitespace around operator tests/test_bias_correction.py:9:121: E501 line too long (141 > 120 characters) tests/test_bias_correction.py:10:17: E225 missing whitespace around operator tests/test_bias_correction.py:10:121: E501 line too long (147 > 120 characters) tests/test_bias_correction.py:11:18: E225 missing whitespace around operator tests/test_bias_correction.py:12:1: W293 blank line contains whitespace tests/test_bias_correction.py:13:12: E225 missing whitespace around operator tests/test_bias_correction.py:14:12: E225 missing whitespace around operator tests/test_bias_correction.py:14:20: W291 trailing whitespace tests/test_bias_correction.py:15:1: W293 blank line contains whitespace tests/test_bias_correction.py:18:121: E501 line too long (143 > 120 characters) tests/test_bias_correction.py:19:73: E231 missing whitespace after ',' tests/test_bias_correction.py:19:121: E501 line too long (204 > 120 characters) tests/test_bias_correction.py:19:130: E231 missing whitespace after ',' tests/test_bias_correction.py:19:132: E231 missing whitespace after ',' tests/test_bias_correction.py:19:153: E231 missing whitespace after ',' tests/test_bias_correction.py:19:156: E231 missing whitespace after ',' tests/test_bias_correction.py:23:73: E231 missing whitespace after ',' tests/test_bias_correction.py:23:121: E501 line too long (204 > 120 characters) tests/test_bias_correction.py:23:130: E231 missing whitespace after ',' tests/test_bias_correction.py:23:132: E231 missing whitespace after ',' tests/test_bias_correction.py:23:153: E231 missing whitespace after ',' tests/test_bias_correction.py:23:156: E231 missing whitespace after ',' tests/test_bias_correction.py:24:73: E231 missing whitespace after ',' tests/test_bias_correction.py:24:121: E501 line too long (204 > 120 characters) tests/test_bias_correction.py:24:130: E231 missing whitespace after ',' tests/test_bias_correction.py:24:132: E231 missing whitespace after ',' tests/test_bias_correction.py:24:153: E231 missing whitespace after ',' tests/test_bias_correction.py:24:156: E231 missing whitespace after ',' tests/test_bias_correction.py:25:1: W293 blank line contains whitespace tests/test_bias_correction.py:27:1: W293 blank line contains whitespace tests/test_bias_correction.py:30:27: E231 missing whitespace after ',' tests/test_bias_correction.py:31:9: F841 local variable 'Scen' is assigned to but never used tests/test_bias_correction.py:33:1: W293 blank line contains whitespace Makefile:246: recipe for target 'pep8' failed make: *** [pep8] Error 1

$ make pep8 Running pep8 code style checks ... tests/test_bias_correction.py:9:121: E501 line too long (143 > 120 characters) tests/test_bias_correction.py:10:121: E501 line too long (149 > 120 characters) tests/test_bias_correction.py:18:121: E501 line too long (143 > 120 characters) tests/test_bias_correction.py:57:9: F841 local variable 'Scen' is assigned to but never used Makefile:246: recipe for target 'pep8' failed make: *** [pep8] Error 1

Undo temp commit b5a3566.

xarray 0.16 already contain what we need. Undo commit a125ff4.

tlvu

@richardarsenault I've resolved all the conflicts with master branch, fixed pep8, removed temporary changes from Trevor.

You can merge this PR when ready (probably wait until Travis-CI is done and is green). Thanks.

tlvu · 2020-07-24T15:53:13Z

@richardarsenault I've deleted branch bias_correction. It's a good habit to always delete the associated branch after the PR is merged to avoid leaving non useful branch behind.

You can create a new branch to continue working on your feature. You can even re-create the same branch name if you wish. At least it'll be starting at the good new location.

richardarsenault · 2020-07-24T16:39:16Z

OK thanks, I am always kind of reticent of deleting branches and stuff so I usually do it a few weeks later to make sure we don't need anything still, but I guess that is futile, isn't it :)

I will take note to delete after merging. Thanks for your patience with me :)

tlvu · 2020-07-24T16:59:21Z

No worries. Git basically uses branches to keep track of commits. If a commit and its ancestors have no branch pointing to it, it'll be eligible to be garbage collected. By merging to master, the master branch will be the one keeping those commits "alive". That's why there is always at least one branch master in any git repository.

From the point of view of us the developers, once we merge to master, we can always navigate the history on master to find the previous branch if needed. And when we want to continue a feature, we should always start from the head of master and not from the head of our previous branch because we risk falling behind master. We always want to be up-to-date with master to integrate with new code from others, to ensure they do not break us and we do not break them.

Hope this further re-assure you that it's safe to delete a dev branch after it has been merged to master or even another branch, as long as there is at least one branch referencing those commits.

richardarsenault · 2020-07-24T17:01:26Z

Sounds good, thanks!

So all new dev branches are built automatically without having to either manually add the branch name to .travis.yml or being forced to create a PR. This creates redundant build on the same commit when PR is created but otherwise no other harms. No redundant build on PR merge since a brand new commit is created for both merge commit or squash commit scenario. See discussion Ouranosinc/raven#292 (comment)

travis-ci: build all branches instead of only master So all new dev branches are built automatically without having to either manually add the branch name to .travis.yml or being forced to create a PR. This creates redundant build on the same commit when PR is created but otherwise no other harms. No redundant build on PR merge since a brand new commit is created for both merge commit or squash commit scenario. See discussion Ouranosinc/raven#292 (comment)

@richardarsenault

…in-autodoc-directive Fix RtD silent build failure in autodoc directive. This PR fixes RtD silent build failure in autodoc directive. The process list https://pavics-sdi.readthedocs.io/projects/raven/en/latest/processes.html is currently empty and it's not supposed to be like that. We never got alerted because it's failing silently. Fixed RtD build: https://pavics-sdi.readthedocs.io/projects/raven/en/test-rtd-build/processes.html and matching RtD build log https://readthedocs.org/api/v2/build/11555558.txt (commit baba22f) Changes: * Turn RtD warnings to build failure so they do not fail silently. * Change Travis-CI build config to also build Epub and Latex doc format to catch RtD failure on Travis-CI before PR is merged. * Various changes to remove all warnings in doc build since warnings will now fail the doc build. Unrelated changes part of this PR (sorry !): * Refresh cookiecutter (first commit in this PR is technically not part of the intended fix of this PR). * Remove Travis-CI directive to only build `master` branch so all dev branch gets build on Travis-CI. Otherwise we either have to create a PR when branch is not even ready or hardcode branch name in `.travis.yml` and have to remember to remove it at the end, very annoying. See comment in #292 (comment) ## Additional Information Matching cookiecutter PR bird-house/cookiecutter-birdhouse#96 @richardarsenault You are probably not interested in the low level details in this PR. I just want to draw your attention to the fact that I found 2 notebooks missing in the toctree (Region_selection.ipynb, Running_models_with_multiple_timeseries_files.ipynb). I added them to the end of the toctree, wonder if you are okay with the ordering or these 2 notebooks were intended to not appear in the doc.

Bias correction

@richardarsenault

…in-autodoc-directive Fix RtD silent build failure in autodoc directive. This PR fixes RtD silent build failure in autodoc directive. The process list https://pavics-sdi.readthedocs.io/projects/raven/en/latest/processes.html is currently empty and it's not supposed to be like that. We never got alerted because it's failing silently. Fixed RtD build: https://pavics-sdi.readthedocs.io/projects/raven/en/test-rtd-build/processes.html and matching RtD build log https://readthedocs.org/api/v2/build/11555558.txt (commit 5521c48) Changes: * Turn RtD warnings to build failure so they do not fail silently. * Change Travis-CI build config to also build Epub and Latex doc format to catch RtD failure on Travis-CI before PR is merged. * Various changes to remove all warnings in doc build since warnings will now fail the doc build. Unrelated changes part of this PR (sorry !): * Refresh cookiecutter (first commit in this PR is technically not part of the intended fix of this PR). * Remove Travis-CI directive to only build `master` branch so all dev branch gets build on Travis-CI. Otherwise we either have to create a PR when branch is not even ready or hardcode branch name in `.travis.yml` and have to remember to remove it at the end, very annoying. See comment in #292 (comment) ## Additional Information Matching cookiecutter PR bird-house/cookiecutter-birdhouse#96 @richardarsenault You are probably not interested in the low level details in this PR. I just want to draw your attention to the fact that I found 2 notebooks missing in the toctree (Region_selection.ipynb, Running_models_with_multiple_timeseries_files.ipynb). I added them to the end of the toctree, wonder if you are okay with the ordering or these 2 notebooks were intended to not appear in the doc.

richardarsenault and others added 14 commits June 15, 2020 11:19

Started updating xclim package dependencies

707b263

add test file for bias correction

e817f7b

Update a few calls and adjust dependencies to new xclim calls

0c57bb4

try and trigger bias_correction branch build

b5a3566

temporary fix top satisfy xclim.sdba requirements of xarray@master

a125ff4

Merge pull request #281 from Ouranosinc/bc_dependency_fix

26d62d1

Update calls and dependencies to support newest xclim version

fixed bias correction function call

a17dc6d

WIP problem with Nans from xarray mean

d0849e7

Bias correction code works but has limitations

3f09f0a

Modifiying start date that causes NetCDF DAP failure

4fbb8df

tests + notebooks: use new official NCML link

e129a39

"birdhouse/1-Datasets" was the first version and won't receive auto NCML deploy update anymore. Will be removed eventually to avoid future confusions.

Merge pull request #291 from Ouranosinc/use-official-ncml-link

66b67e3

tests + notebooks: use new official NCML link "birdhouse/1-Datasets" was the first version and won't receive auto NCML deploy update anymore. Will be removed eventually to avoid future confusions.

richardarsenault requested a review from tlvu July 22, 2020 18:35

Merge remote-tracking branch 'origin/master' into bias_correction

dc53a33

tlvu reviewed Jul 22, 2020

View reviewed changes

tlvu added 4 commits July 22, 2020 16:08

.travis.yml: remove branch bias_correction from build branch

a08cc05

Undo temp commit b5a3566.

.travis.yml: remove xarray install from source

11b7249

xarray 0.16 already contain what we need. Undo commit a125ff4.

tlvu approved these changes Jul 24, 2020

View reviewed changes

richardarsenault merged commit adc2eb4 into master Jul 24, 2020

tlvu deleted the bias_correction branch July 24, 2020 15:48

tlvu mentioned this pull request Jul 24, 2020

Fix RtD silent build failure in autodoc directive #293

Merged

tlvu mentioned this pull request Jul 28, 2020

travis-ci: build all branches instead of only master bird-house/cookiecutter-birdhouse#98

Merged

Zeitsperre pushed a commit that referenced this pull request Aug 17, 2023

Merge pull request #292 from Ouranosinc/bias_correction

d239a46

Bias correction

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bias correction #292

Bias correction #292

richardarsenault commented Jul 22, 2020

richardarsenault commented Jul 22, 2020

tlvu left a comment

tlvu Jul 22, 2020

Zeitsperre Jul 23, 2020

tlvu Jul 24, 2020

tlvu Jul 22, 2020

Zeitsperre Jul 23, 2020

tlvu left a comment

tlvu commented Jul 24, 2020

richardarsenault commented Jul 24, 2020

tlvu commented Jul 24, 2020

richardarsenault commented Jul 24, 2020

Bias correction #292

Bias correction #292

Conversation

richardarsenault commented Jul 22, 2020

richardarsenault commented Jul 22, 2020

tlvu left a comment

Choose a reason for hiding this comment

tlvu Jul 22, 2020

Choose a reason for hiding this comment

Zeitsperre Jul 23, 2020

Choose a reason for hiding this comment

tlvu Jul 24, 2020

Choose a reason for hiding this comment

tlvu Jul 22, 2020

Choose a reason for hiding this comment

Zeitsperre Jul 23, 2020

Choose a reason for hiding this comment

tlvu left a comment

Choose a reason for hiding this comment

tlvu commented Jul 24, 2020

richardarsenault commented Jul 24, 2020

tlvu commented Jul 24, 2020

richardarsenault commented Jul 24, 2020