-
Notifications
You must be signed in to change notification settings - Fork 9
Draxler Turbulence Model #20
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
base: main
Are you sure you want to change the base?
Conversation
src/pyelq/meteorology.py
Outdated
| horizontal direction [deg or m/s] | ||
| wind_turbulence_vertical (np.ndarray, optional): Parameter of the wind stability in | ||
| vertical direction [deg] | ||
| vertical direction [deg or m/s] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest having the wind_turbulence_horizontal_deg, wind_turbulence_vertical_deg, wind_turbulence_horizontal_meter_per_sec (wind_turbulence_u_component) and wind_turbulence_vertical_meter_per_sec (wind_turbulence_v_component) as the properties of the meteorology object. I would also suggest implementing a method in the meteorology object to calculate the horizontal and vertical turbulences in m/s (similar to calculate_wind_turbulence_horizontal). I think this will make it clearer and we can use either option based on the need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made this update in meteorology.py. There are now a lot of places where references should be updated, I will go through and update these but thought I'd push my changes so far anyway.
I have also implemented the horizontal and vertical wind turbulence parameter calculation functions. The perceived benefit of this turbulence model is a better estimate of turbulence using the HF wind (though we have seen similar results at 1Hz and 10Hz with this model). Since the current implementation of Meteorology only has one time axis, I propose that to use HF wind in this way, you could create an HF meteorology object and calculate the parameters there, then resample and assign to the LF met object.
I realise that this is a bit of an awkward workflow, but I think that perhaps the HF use case is quite niche, and so we shouldn't clutter the meteorology class with dual time axis artifacts. I would be interested to hear your thoughts.
| DEFAULT_DRAXLER_HORIZONTAL = { | ||
| "scale_ground": 0.9, | ||
| "scale_air": 0.9, | ||
| "exp_ground": 0.5, | ||
| "exp_air": 0.5, | ||
| "t_i_ground": 300.0, | ||
| "t_i_air": 1000.0, | ||
| } | ||
| DEFAULT_DRAXLER_VERTICAL = { | ||
| "scale_ground": 0.9, | ||
| "scale_air": 0.945, | ||
| "exp_ground": 0.5, | ||
| "exp_air": 0.806, | ||
| "t_i_ground": 50.0, | ||
| "t_i_air": 100.0, | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at the Draxler paper where you take the default parameters. I suggest referring to the table or the equation in which you obtained these values in the documentation.
I think it would be also beneficial to explain what each parameter means.
If one wants to override these default parameters, what would be the source to obtain them?
How sensitive are the estimated sigma_hor and sigma_ver to these input parameters in case one wants to use these default values instead of those dependent on the location and the ground threshold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at this, a lot was really unclear. This is definitely not my specialist subject and Jason has not been around today to fill in any gaps but I've made an attempt at providing clarity based on what I can understand of the Draxler paper. I would appreciate your feedback.
In regards to your final points, I'm not sure I could recommend another source of parameters to use. There is this paper which we have also used for reference material, but seems to have less thoroughly defined parameters for the universal functions. (pp27)
Again, I am unsure about the sensitivity of the parameters to the sigma values, but I have attempted to provide some explanation to this effect, though it is not particularly rigorous.
| """ | ||
|
|
||
| source_map: SourceMap | ||
| turbulence_model_horizontal: TurbulenceModel = field(default_factory=AngularModel, init=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a minimal, reproducible code to the pull request as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have included the necessary changes to the example notebook in the PR description.
… turbulence in m/s.
|
Hi Rudy, Given your comment regarding gaussian_plume.py about passing arrays which could be different units I feel it might be a bit confusing to upate all the inner plume functions with more potential parameters which specify if it's degrees or m/s, leaving it like it is now is also a bit strange I think as it feels like mix of notation. So, I think the implementation you had before where the parameter was named the same only the unit might be different was a better way after all. Would it be possible to revert back to that commit, hence not have meter_per_sec or deg in the parameter names. I think this also ensures backwards compatibility and would be preferred from my perspective. Thanks. Tagging @TannazH for information. |
Hey Bas, Thanks for the feedback, that sounds good to me. Would you prefer if I revert to that commit? Or strip out the unit-specific bits but keep the documentation and other changes I made? |
|
I think it's probably easiets to strip out the unit specific bits. I really like what you did in the latest commit (Draxler Turbulence model, cbf5c54) with the turbulence model class, but probably the edits from the commit before that (Split turbulence vectors..., 6b3fe6c) need to be reverted. However going back to the commit before that also undo the latest edits which I would suggest to keep. Long story short: probably stripping the unit specific bits is the easiets. However, I'll leave it up to you what you prefer! |
Description
TurbulenceModelbase class to define difference turbulence behaviours.AngularModel.sigma_hor/sigma_vertcalculation out ofcompute_coupling_arrayduring the forward model calculation and calculates the plume stability from the turbulence model in use.Issues:
I am not sure on the fact thatThis is arguably now a breaking change since the API ofwind_turbulence_horizontalnow can represent values in either deg or m/s, but did it this way to avoid savingturbulence_degandturbulence_msin separate arraysMeteorologyhas changed.meteorology.py still needs the function to calculate the turbulence in m/s. We calculate from high res wind and store as low res. Not sure how the use cases of this library would dictate the implementation of this.gaussian_plume.pystill passes round one array for each of the wind turbulence parameters, giving us one array representing two units again. After the interpolation step, the turbulence is passed around as a arrays instead of inside the met object, so we'd be adding two more parameters to all of the 'inner' GP calculation functions. I will wait to see what the consensus is on this before proceeding.Type of change
Please delete options that are not relevant.
Jupyter Notebooks
This model can be run with the basic Jupyter notebook included in the repo with the following changes:
Meteorology definition:
Gaussian Plume definition:
How Has This Been Tested?
gaussian_plume.py, there is definitely some more testable code now. I will add tests if you think this is the correct approach!Checklist: