-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add utilities for floris models #840
Conversation
Architecturally, @paulf81 what do you think of putting |
@paulf81 , agree with Raf's comments above. Also for reference, in this commit (cbd3cf8), I removed a slightly different version of this same functionality where the list of keys was optional, and if not provided, the object would be scraped for all attributes and those would be returned as a dictionary. Could be worth including if you think it is useful. |
Done! |
Sounds interesting! Looking at the code I wasn't quite positive which function should be added back in, and then I think some of these will need an update to the v4 standards, do you want to try to add into this PR? Or a seperate one? |
Hey @paulf81 , I think @rafmudaf was suggesting moving the |
@misi9170 , I added some code for getting/setting the power_thurst_model, and somehow it doesn't work. It may be this thing of how the turbine_type can be stored in a few possible formats but I was wondering if you might have a look at these new codes Monday and see if you spot my error? I added two simple tests: and this one is failing:
|
@paulf81 , @misi9170 I went ahead and added this functionality and test to my PR to your branch Paul in this commit: paulf81@afad340. I changed it so that is uses the currently loaded turbine type, updates the |
Move FlorisModel realted utilities onto FlorisModel class
Thank you @bayc , I merged your pr into mine so those changes are now reflected in this pull request |
@rafmudaf this is now done, following the merge of @bayc code |
Those look fine to me. For some reason, @paulf81 , I can't see the history of what you had implemented earlier that wasn't working? But perhaps the fix was to add the |
I think that's now moot, it was the change power-thrust functions I had written, but Chris' versions don't seem to suffer from this problem |
Overall this looks fine to me, but so that we have a record and some guidelines for future development, what is the logic behind the reorganization of the The only objection I have is that I would have kept |
@bayc would you mind to comment here? |
It's subtle, but I also suggest to put |
I definitely like the idea of having the different methods grouped. I've reorganized them again slightly and pushed up the change (which we can certainly revert), so that now we have:
Regarding the ordering I've chosen for the setting and running:
Regarding the ordering I've chosen for extracting turbine performance after running
|
That organization works for me @misi9170 ! |
Add utilities for floris models
co-authored with @bayc
This pull request: