Hydrolib core update

[veenstrajelmer]

Issue addressed

Fixes #130
Might also fix 111 >> to be checked

Explanation

This PR entails an update for hydrolib-core calls to be able to work with hydrolib-core 0.8.0

Checklist

• Updated tests or added new tests >> seems not relevant in this case
• Branch is up to date with main
• Tests & pre-commit hooks pass
• Updated documentation if needed >> Not required
• Updated changelog.rst if needed >> mentioned that dependencies were updated

Status: several sources for failing tests have been resolved, see issue #130 for an overview. There is only the model validation that fails (mod0._test_equal gives errors). This was worked around by updating some mdu keyword values in the two mdu files, but these values are not always realistic. Let's discuss this.

Furthermore, meshkernels contacts_set was now used somewhere in the code. The old method does not work anymore, but is also used elsewhere in the code. This does not raise errors, since it is not covered by tests, which is quite tricky.

[hboisgon]

Thanks for this @veenstrajelmer !
Indeed no need to update the docs or changelog for internal changes. Maybe when we release in the main message it may be nice to mention that several dependencies versions have been updated but for now no need.

For your remaining issue with the mdu file, I'm not sure what is the best way to go. I guess you mean the values with -999? Does hydrolib-core have defaults for these? We can then use the same. Or should the line somehow just be deleted? Or should we come up with our own default somehow or allow the user to add that value in the linked setup function?

[veenstrajelmer]

Indeed, I meant the -999 and the . values. The -999 values are hydrolib-core default values. Removing the keywords still gives an error unfortunately, I think this is because of a discrepancy in Hydrolib-core but I will have to check this with @tim-vd-aardweg. The . I do not understand, since the hydrolib-core defaults are "". But again, empty values (or removing the keyword) causes the error. Not sure from which package this comes from. I could use a unit test here since I cannot easily debug the current test (and it is also quite slow).

Furthermore, I implemented usage of contacts_set but I saw at least one other part of the code where this should be done. This raises no error since it is not covered by tests yet (as is 30% of the code). It could well be that we are forgetting important parts of the code here, also with the previous PR. So it would be amazing if the test coverage could be increased.

[veenstrajelmer]

-999 and . were changed in Deltares/HYDROLIB-core#661 (comment). Consider reverting this or resolving the issue differently. For -999 we could just update the expected values (although an empty value might be preferred). The behaviour of . is unexpected and documented more thoroughly in Deltares/HYDROLIB-core#703

[shartgring]

Looks good. As Hélène mentioned in PR133, I should check if there are no conflicts. So I will address those after this one has been merged into main.

Personally I am not aware of the implications of changing the default values and the differences between . and "", and when to use -999, so either are fine for me. I think given the timeline with the DSD, it is nicer to give priority to getting the package version of hydrolib-core up to the newer version, and create a new issue for the mdu default values. That way, we can also address future improvement this week in an enviroment with all packages as up to date as possible

[veenstrajelmer]

I agree, I have created issue #148 to update the expected values. The only thing left is add the meshkernel contacts_set elsewhere in the code

[sonarcloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud