Wflow.jl icon indicating copy to clipboard operation
Wflow.jl copied to clipboard

Published 20 hours ago • verified publisher icon Deltares

Replace use of `LoopVectorization` in local inertial model `stable_timestep` function with `Polyester` (`reduction`)

Open verseve opened this issue 10 months ago • 5 comments

Issue addressed

Fixes #360

Explanation

The internal time step of the local inertial model (stable_timestep function) can get zero when LoopVectorization is applied (@tturbo) to the for loop of these functions. This issue occurred on a virtual machine, Windows 10 Enterprise, with Intel(R) Xeon(R) Gold 6144 CPU (2 processors). This has been fixed by replacing @tturbo with reduction of the Polyester package.

Checklist

  • [ ] Updated tests or added new tests
  • [x] Branch is up to date with master
  • [x] Tests & pre-commit hooks pass
  • [x] Updated documentation if needed
  • [x] Updated changelog.md if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

verseve avatar Apr 16 '24 14:04 verseve

@JoostBuitink: the Blue Nile model (1d-2d local inertial model) ran fine with these code changes on a WCF node. I think it would be good to also test this for one of your wflow model applications on a WCF node, as part of the review.

verseve avatar Apr 17 '24 12:04 verseve

Marked this PR as draft, because I just thought of another solution without the need to replace tturbo from LoopVectorization. The wflow Blue Nile model is running again to test this.

verseve avatar Apr 18 '24 09:04 verseve

I did a couple of checks with the model that produced errors, and this branch seems to indeed fix it! @shartgring will also do a check, since he just ran into the issue that should be solved by this PR

JoostBuitink avatar May 08 '24 06:05 JoostBuitink

I did not get the error myself anymore when redoing my runs so that looks good!

At the same time, the Wflow model I recieved from @tbuskop still shows the error. However, that one uses kinematic-wave instead of local-inertial. So, it might be the question whether this PR will solve the error mentioned in the issue. I will elaborate further in the issue itself

shartgring avatar May 08 '24 13:05 shartgring

At the same time, the Wflow model I recieved from @TBuskop still shows the error. However, that one uses kinematic-wave instead of local-inertial. So, it might be the question whether this PR will solve the error mentioned in the issue. I will elaborate further in the issue itself

I did check the wflow model of Ted, and the forcing file is not valid. At 2015-11-02 infinite values occur for a couple of grid cells for variable pet. This is causing the wflow error.

verseve avatar May 08 '24 20:05 verseve

Just re-encountered this type of error, so I can also provide the model for further testing (loc iner. 1d floodplains for Rio). We need to merge this branch soon, this error really limits our setups as in most of them floodplains are included.

atsiokanos avatar May 14 '24 14:05 atsiokanos

Just re-encountered this type of error, so I can also provide the model for further testing (loc iner. 1d floodplains for Rio). We need to merge this branch soon, this error really limits our setups as in most of them floodplains are included.

@JoostBuitink: could you please add your review?

verseve avatar May 14 '24 14:05 verseve

Just re-encountered this type of error, so I can also provide the model for further testing (loc iner. 1d floodplains for Rio). We need to merge this branch soon, this error really limits our setups as in most of them floodplains are included.

@JoostBuitink: could you please add your review?

Oops, my apologies. I thought I already approved the PR, but it looks like I only left a comment. Should be approved now!

JoostBuitink avatar May 14 '24 15:05 JoostBuitink