Deltares / hydromt_delft3dfm

Delft3D FM plugin for HydroMT
https://deltares.github.io/hydromt_delft3dfm/
GNU General Public License v3.0
9 stars 2 forks source link

Hydrolib core update #139

Closed veenstrajelmer closed 2 months ago

veenstrajelmer commented 2 months ago

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

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.

veenstrajelmer commented 2 months ago

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 commented 2 months ago

-999 and . were changed in https://github.com/Deltares/HYDROLIB-core/issues/661#issuecomment-2208930830. 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 https://github.com/Deltares/HYDROLIB-core/issues/703

veenstrajelmer commented 2 months ago

I agree, I have created issue https://github.com/Deltares/hydromt_delft3dfm/issues/148 to update the expected values. The only thing left is add the meshkernel contacts_set elsewhere in the code

sonarcloud[bot] commented 2 months ago

Quality Gate Passed 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