Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
May 14, 2024

Conversation

verseve
Copy link
Contributor

@verseve verseve commented Apr 16, 2024

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
  • Branch is up to date with master
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.md if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

vers-w added 3 commits April 16, 2024 16:07
Polyester v0.7.10 is required for using `reduction` of an already initialized variable.
…mestep` function

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 occured on a virtual machine, Windows 10 Enterprise, with Intel(R) Xeon(R) Gold 6144 CPU (2 processors). This has been fixed by replacing @tturbo by `reduction` of the `Polyester` package.
@verseve verseve marked this pull request as ready for review April 17, 2024 11:55
@verseve
Copy link
Contributor Author

verseve commented Apr 17, 2024

@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 verseve marked this pull request as draft April 18, 2024 05:46
@verseve
Copy link
Contributor Author

verseve commented Apr 18, 2024

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 verseve marked this pull request as ready for review April 22, 2024 07:30
@JoostBuitink
Copy link
Contributor

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

@shartgring
Copy link
Collaborator

shartgring commented May 8, 2024

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

@verseve
Copy link
Contributor Author

verseve commented May 8, 2024

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.

@atsiokanos
Copy link

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.

@verseve
Copy link
Contributor Author

verseve commented May 14, 2024

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?

@JoostBuitink
Copy link
Contributor

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!

@verseve verseve merged commit 00be215 into master May 14, 2024
9 of 10 checks passed
@verseve verseve deleted the fix_LV_LIE_stable_timestep branch May 14, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wflow_sbm run fails with local inertial routing (virtual machine)
5 participants