77 setup 1d laterals

[xldeltares]

Issue addressed

Fixes #77

Explanation

Explain how you addressed the bug/feature request, what choices you made and why.

Checklist

• Updated tests or added new tests
• Branch is up to date with main
• Tests
• pre-commit hooks pass --> in a seperate issue
• Updated documentation if needed
• Updated changelog.rst if needed
• update model api
• update boundares.py when could not reproject for geodataset hydromt#613 is resolved. --> wait for the next release

Additional Notes (optional)

Add any additional notes or information that may be helpful.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
1.7% Duplication

[xldeltares]

Hi @hboisgon , I have finalized the 1D lateral branch needed for PUB. could you please review it? Thanks!

[hboisgon]

Nice work @xldeltares! I have some suggestions for geodataset objects especially in the workflow before approving and merging.

I still wonder if separating functions for boundaries and laterals into constant and timeseries (geodataset) would not make things more clear both for user and developper? If so can be tackled here or open an issue for later.

While reviewing I also think we might want to open some issues in core. They wont be solved and released fast I guess so we can keep what we have here as well.

[hboisgon]

Does this happen? Now that I think I think I had the issue with wflow. Might be an idea to have better support in core then ie an option to raise error when reading if time mismatch or no error but return a flag or dataset actual (start, end) tuple. What do you think?

[xldeltares]

I think it is mainly because the datetime parsing in case of a csv. It could easily go wrong. In that case, we issue an error and ask explicitly for yyyy-mm-dd HH:MM:SS.

[hboisgon]

Csv or netcdf maybe still an issue for core? That if a time_tuple is passed to any of the get_data we should do an extra check in core to make sure the slicing happened?

[xldeltares]

Yes, true! I added this request to hydromt issues.: Deltares/hydromt#665

[hboisgon]

I had a look at Deltares/hydromt#613 and fixed it in a PR. Good catch! Not sure about when it would be released though so we may have to keep your workaround (if any) longer and open an issue to remind us to come back to it when hydromt is released again (shame it was not in the release from last week).

[xldeltares]

HI @hboisgon , thanks for the nice review and feedback! I agree with your comments on the current way of setting up forcing. I think that should be a more general discuss, therefore, a new issue has been created: #117 . It is a quite big issue and several sub issues can be created from it, but for now I kept it as one big issue. Hope that we could pick it up earlier next year!

Could you re-review this PR (hopefully an easy approve) and also check if you agree with #117?

note that the sonarcloud code is not passing because of two critical cognitive complex functions write_1dlateral and write_geoms. But I could not think of an easy way to address it yet so I left them as they are now.

[hboisgon]

Hi @xldeltares , thanks for the changes! Looks good to merge now.

I think that the quality gate is failing because of code duplication (should be <= 3%).
It was at 4.8 and I now reduced to 3.1 so almost there...
Can you check my changes? Especially for the new _read_forcingfile function. For each forcing boundary/meteo etc there was a lot of duplicates that I simplified into one function. I needed to add a lot of flags (true/false). Could you check that these flags are indeed needed or actually in some cases we did not do it correctly? Also for read_meteo I actually changed the behavior (set factor and offset to 0 and 1 instead of reading from df forcing as I think when reading we do the conversion so the new values should be 0 and 1 - you can compare now and either from main or previous commit version)

If all looks good we can merge and for the remaining 0.1% duplication, I leave it up to you if it is worth the effort

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

No Coverage information
0.3% Duplication

[xldeltares]

@hboisgon , nice changes! I did similar for the rest of the duplication so we are now green! will merge the pull request and close the branch.