Fitch wind farm scheme bug fix: axial induction factor#2242
Fitch wind farm scheme bug fix: axial induction factor#2242weiwangncar merged 4 commits intowrf-model:developfrom
Conversation
|
@SengersB Thank you very much for your contribution to the WRF model. Your code has many many differences from the develop branch in our repository. For example, your LICENSE.txt file is different from our version. There maybe many more modified files (out of 267 files) are not part of your bug fix. Please clone the latest WRF model code, and add only the code that pertaining to the bug fix to this PR. |
|
@weiwangncar Thanks for the quick response. I used the branch |
|
@SengersB Starting from WRF v4.7.1 is good. But did you really changed 267 files to implement your Fitch fix? It is unlikely. You should identify the files changed in your original code, and add them to the new release. If you started with WRFv4.7.1, the changes that are already in that version should not show up as updates. See this page for information on how to make code changes. |
|
@SengersB Is your intention to contribute your modification to our repository? Or do you plan to maintain the code in your own branch? |
|
@weiwangncar I have started over fresh from release-r4.7.1 made sure my modification were added. My intention is to contribute the modification to your repository, so to be part of the next (bugfix) release. We are not maintaining the code ourselves. |
|
@SengersB It still says 'Files changed 278'. See the last item on top of your PR message. |
|
@weiwangncar That is compared to your branch wrf-model:develop. Should I use this branch as a base? |
|
@SengersB Our master and develop branches are the same. You should first clone the WRF repository code (make sure that code is version 4.7.1 by checking the top level README file), and then add your changes in those five files to the cloned code. Please see the page I provided early for instructions. |
1a7a185 to
c43caff
Compare
|
@weiwangncar Alright turns out I just had to rebase. Pending clarification on the LICENSE.txt, I hope this is what you need to review and merge. |
|
@SengersB Thanks, and this is what we expect to see! The regression test has passed too: |
|
@SengersB The remaining issue is the text for license which we will have to come out of way to deal with it. |
|
@weiwangncar Just checking in: is there anything I need to do? |
|
@SengersB We would like to consult our lawyers to see where we can insert your proposed license. |
dudhia
left a comment
There was a problem hiding this comment.
code looks safe to add - only adds a diagnostic
|
@weiwangncar did we resolve what to do with the license? |
|
@islas Go ahead and make the change the developer agreed to. |
|
@SengersB I am unable to push the fixes we discussed. Could you make sure you have the following enabled?
|
|
@SengersB I'm not sure. I've never encountered that issue. Perhaps we can try this: Here is a patch file with the edits that I did. You can download the file and apply it to your branch. From there you can commit & push the changes. You can apply using |
|
@islas Thanks, I'll make the changes. @weiwangncar @dudhia The last days we were advertising to some other heavy users of the wind farm parameterizations that this bug fix will hopefully be in the next WRF release. They expressed the desire to keep the original formulation (without this proposed bugfix) also available. I therefore propose to implement the bugfix as an addition option (default) in windfarm_opt. |
|
I enncourage keeping the previous code as an option. I had misread this as just providing a new diagnostic thrust, but now I see it alters results. It can be done within the same PR. |
f532d2c to
1b01144
Compare
|
Hi @dudhia @weiwangncar, I added the discussed changes and pushed the updated code. In short:
Again apologies for the mess with the licensing. Hope everything is fine now and we can merge this bugfix after your checks. Thanks, Balthazar |
|
OK, we need to check which automated tests failed and get back to you. |
Registry/Registry.EM_COMMON
Outdated
| package fitchscheme windfarm_opt==1 - state:power | ||
| # THE FOLLOWING LINES OF CODE ARE LICENSED UNDER doc/licenses/fraunhofer_license.txt | ||
| package fitchscheme windfarm_opt==1 - state:power,thrust | ||
| # END LICENSE |
There was a problem hiding this comment.
@SengersB May need a separate package line just for the variable 'thrust', since 'power' is part of the original scheme.
There was a problem hiding this comment.
Agreed. I don't know the syntax on how to do this here. If this is an issue, I'd be okay with removing the licensing statement here
|
@SengersB Your latest changes have some compilation errors: |
|
@weiwangncar Apologies, I fixed these compilation errors now. Unfortunately I cannot run a full test on this code, as v4.7.1 does not want to compile on our cluster and we're currently without IT support. So I had to make these changes in v4.6.1 and transfer them, but apparently missed something during the merging. I apologize again |
|
@SengersB Thanks for the fix. Now the regression test results look good: |
|
@SengersB Can you address my comment for your registry change? |
|
@weiwangncar I have now removed the licensing statement around this one line |
Bug fix and very small dev in Fitch wind farm parameterization
TYPE: bug fix
KEYWORDS: fitch, wind farm parameterization, turbine, wind farm, wind energy
SOURCE: Balthazar Sengers, Fraunhofer IWES, Germany
DESCRIPTION OF CHANGES:
Problem:
Solution:
LIST OF MODIFIED FILES:
M Registry/Registry.EM_COMMON
M dyn_em/module_first_rk_step_part1.F
M phys/module_pbl_driver.F
M phys/module_wind_fitch.F
A doc/licenses/fraunhofer_license.txt
TESTS CONDUCTED:
RELEASE NOTE:
Fitch wind farm parameterization: correct local (grid cell) wind speed to free wind speed using axial induction correction proposed by Vollmer et al. (2024) https://doi.org/10.5194/wes-9-1689-2024