Switch groundwater sign

stijnvanhoey commented 2 months ago

See https://github.com/inbo/niche_vlaanderen/issues/319 @geohydrodata updated the data files and executed the required adjustments to the code, ready to test.

Note this will require a breaking change when releasing.

johanvdw commented 2 months ago

To test in a separate environment:

conda create --name niche_dev python=3.11
activate niche_dev
conda install pandas pyyaml rasterio fiona git geopandas matplotlib jupyter
pip install git+ssh://git@github.com/inbo/niche_vlaanderen.git@DD-groundwater-sign

cecileherr commented 1 month ago

A small correction of a typing error in the name of the branch (underscore)

conda create --name niche_dev python=3.11
activate niche_dev
conda install pandas pyyaml rasterio fiona git geopandas matplotlib jupyter
pip install git+ssh://git@github.com/inbo/niche_vlaanderen.git@DD-groundwater-sign

should be

pip install git+ssh://git@github.com/inbo/niche_vlaanderen.git@DD-groundwater_sign

stijnvanhoey commented 1 month ago

A small correction of a typing error in the name of the branch (underscore)
conda create --name niche_dev python=3.11
activate niche_dev
conda install pandas pyyaml rasterio fiona git geopandas matplotlib jupyter
pip install git+ssh://git@github.com/inbo/niche_vlaanderen.git@DD-groundwater-sign
should be

pip install git+ssh://git@github.com/inbo/niche_vlaanderen.git@DD-groundwater_sign

If ssh is not working, you can try

pip install git+https://github.com/inbo/niche_vlaanderen.git@DD-groundwater_sign

cecileherr commented 1 month ago

Many thx for the changes!

Not checked the code yet, but looked at test cases

Testing this first version of niche_vlaanderen 2 with my reference test dataset (aka FakeComplete, for further reference), for 3 scenario's, respectively:

a simple model only based on soil+mhw+mlw,
a full model,
a full model with posthoc limitation of the results with management and inundation (that is specifying set_input => "management_vegetation"+ "inundation_vegetation" )

Results until now

the output files (vegetation predictions, e.g. mymodel_V27_detail.tif or mymodel_V27.tif ) seem to be OK in all scenario's: same result as in v 1 (contrary to what I told Sacha earlier this week)
the abiotic files (mymodel_acidity.tif & mymodel_nutrient_level.tif) seem to be OK
the deviation files (e.g. mymodel_mlw_19.tif and mymodel_mhw_19.tif) seem to have the same value as with niche_vlaanderen v 1.x but with a reverted sign (as expected given the reverted sign of the inputs)

Possible improvements

deviation files: it might be nice to use the same sign convention as with niche_vlaanderen v 1.x so the user doesn't have to change the way he interprets the output files and so he can use the same legend layers as before (lyr, sld, ..) (in other words: the sign of groundwater levels as input layers would change in v 2 but the sign of the deviation layers - the output files - would stay the same as in v1.x)
I have the impression that the colour scale for the groundwater level plots (plots of the input layers, for instance simple.plot("mlw")) is still the same as in the previous version, at least in my example. Since the scale is asymmetrical (we expect many more pixels with groundwater levels under the ground than above ground) the result could be better (see printscreen, new dev version left)

Any thoughts on this?

geohydrodata commented 1 month ago

Many thx for the changes!

You are welcome!

Possible improvements

* deviation files: it might be nice to use the same sign convention as with `niche_vlaanderen v 1.x` so the user doesn't have to change the way he interprets the output files and so he can use the same legend layers as before (lyr, sld, ..) (in other words: the sign of groundwater levels _as input layers_ would change in v 2 but the sign of the deviation layers - _the output files_ - would stay the same as in v1.x)
* I have the impression that the colour scale for the groundwater level plots (plots of the input layers, for instance `simple.plot("mlw")`) is  still the same as in the previous version, at least in my example. Since the scale is asymmetrical (we expect many more pixels with groundwater levels under the ground than above ground) the result could be better (see printscreen, new dev version left)

OK, I will look into this. We must make sure that plotting then doesn't conflict with a sign change in one case, and no sign change in the other.

geohydrodata commented 1 month ago

Possible improvements

deviation files: it might be nice to use the same sign convention as with niche_vlaanderen v 1.x so the user doesn't have to change the way he interprets the output files and so he can use the same legend layers as before (lyr, sld, ..) (in other words: the sign of groundwater levels as input layers would change in v 2 but the sign of the deviation layers - the output files - would stay the same as in v1.x)

The mhw, mlw deviation output files have changed sign as requested. Implementation (and update of according tests) in https://github.com/inbo/niche_vlaanderen/commit/8bc81541a6d3361e0a9cbfc92959bb05d5cecc25.

geohydrodata commented 1 month ago

I have the impression that the colour scale for the groundwater level plots (plots of the input layers, for instance simple.plot("mlw")) is still the same as in the previous version, at least in my example. Since the scale is asymmetrical (we expect many more pixels with groundwater levels under the ground than above ground) the result could be better (see printscreen, new dev version left)

This has been addressed in https://github.com/inbo/niche_vlaanderen/commit/b7527c698caa5ff237cf8c7305be9d2fcc3ae226

stijnvanhoey commented 2 weeks ago

@cecileherr, @geohydrodata updated this PR with the requested improvements on the sign changes of groundwater and the reference table integration with Zenodo. We did an effort to standardize the system-tables as much as possible (providing a 'description' and 'beschrijving' column) and by using the column names of the niche_vegetation into the other system tables, clarifying the connection between both.

Furhtermore, the procedure of the update when a new version in zenodo is published is included in the documentation:

clarify to users how to compare both
Add to the release documentation which adjustments need to be done

Note We also added a chart to illustrate the interconnection between the system tables -

Can you review and test the code again to make sure everything is adjusted correctly? If ok, everything is good to go for the version v2.0.

cecileherr commented 5 days ago

I tried the updated version on a few test cases (unfortunately no time yet to check the code itself)

Testing this second version of niche_vlaanderen 2 with my reference test dataset (aka FakeComplete, for further reference), for different scenarios, respectively:

a simple model only based on soil+mhw+mlw,
a full model,
a full model with posthoc limitation of the results with management and inundation (that is, specifying set_input => "management_vegetation" + "inundation_vegetation" )
a full model with a custom niche table,
a full model with a custom acidity layer
a niche delta for the difference between the results of a simple and a full model
a validation of the results of the full model with a vegetation map

I ran this both in niche 1.3 and niche dev (2) and compared the results: good news, I could not find differences in the output 👍

The deviation files (e.g. mymodel_mlw_19.tif and mymodel_mhw_19.tif) now have the same sign in version 2 as in niche 1.3 as requested: thx! And the adapted colour scale for the groundwater level plots is fine! (as requested in https://github.com/inbo/niche_vlaanderen/pull/376#issuecomment-2137404453)

Nice that the version of the niche table now appears in the created log file!

# Niche Vlaanderen version: 1.3 (= dev version for niche_vlaanderen 2)
# Using latest niche_vlaanderen  1.3
# Reference values:
#     version: 12C
#     source: 10.5281/zenodo.10417821
#     original file: NICHE_FL_referencegroundwaterlevels_v12C.csv
# Run at: 2024-07-04 14:24:56.985587

But one remark/question: 10.5281/zenodo.10417821 is the source for all the versions (thus by default the latest published version). Should we refer explicitely to the used version instead of the whole dataset (in this case 12C thus 10.5281/zenodo.10521548)?

And a last comment: there is a new warning when I use mymodel.plot("mhw"):

C:\Users\cecile_herr\.conda\envs\niche_dev\Lib\site-packages\IPython\core\pylabtools.py:77: DeprecationWarning: backend2gui is deprecated since IPython 8.24, backends are managed in matplotlib and can be externally registered.
  warnings.warn(

Maybe I can post it as a new issue for later?

Many thx for the changes!

stijnvanhoey commented 1 day ago

But one remark/question: 10.5281/zenodo.10417821 is the source for all the versions (thus by default the latest published version). Should we refer explicitely to the used version instead of the whole dataset (in this case 12C thus 10.5281/zenodo.10521548)?

Very good point, I updated the reference and the documentation.

stijnvanhoey commented 1 day ago

there is a new warning when I use mymodel.plot("mhw"): C:\Users\cecile_herr.conda\envs\niche_dev\Lib\site-packages\IPython\core\pylabtools.py:77: DeprecationWarning: backend2gui is deprecated since IPython 8.24, backends are managed in matplotlib and can be externally registered. warnings.warn( Maybe I can post it as a new issue for later?

This seems to be a deprecation of either Ipython/matplotlib versions when running the code inside a notebook. I added a new issue, see https://github.com/inbo/niche_vlaanderen/issues/380

inbo / niche_vlaanderen

Switch groundwater sign #376