Print error when turbine models are missing reference values#647
Conversation
|
Just heads up a very similar error message was recently removed in #603 (see this line). In my opinion, this sort of messaging belongs either in the documentation or in a very high level, user facing area of the code. Doing input validation in various places throughout the low level code adds noise to the context of the low level functionality and makes tracking these things down more difficult. You also know my opinion about printing these sorts of things to the console - it should be used very sparingly because even a little too much information and it just gets lost in excess output. A good smell test is to consider what the function called I suggest either removing this in favor of an API change page in the online documentation or including these checks in a high level portion of the code where other similar input validation is happening. One place we've been aggregating these is in the FlorisInterface.init routine. |
|
These are good comments @rafmudaf I'll make some changes and resubmit |
|
hi @rafmudaf I was trying to put it in the init routine, but it seems like it is going to get bulky, since the user passes in the input yaml, which references the turbine yaml, I think I would have to do a shadow load to know the error is triggered within the second line of the init function so there isn't a chance to catch it here, I will think some more about it, but I think it will be tough to put in floris_interface |
|
ok! have a look now @rafmudaf ? I couldn't move it all the way up to floris interface, but I could go up to floris.py, and I tried to not muddle any exisiting functions by making this step clearly it's own thing. I know I'm still using a console output, but my thinking is a console error will happen if I don't do this to, so maybe this solution is splitting the difference? |
|
@paulf81 I've made a few changes here. Primarily, instead of triggering an error and catching it, I'm testing whether the key exists in the turbine dictionary. Then, the error messages are aggregates and sent in one one error message. I also added the turbine name. This allows for one message to include all things missing from the input file. One thing we could add is to aggregate these for all turbines, so it would mean raising the error outside of the for-loop. That might be useful, and it's also easy to do. I can go either way on that. I also added a page in the documentation to describe the turbine input file and pointed to that in the error message that is raised. |
|
I think these changes are a big improvement, thank you!! |
|
The two errors are reported like this: |
Add informative error about new input requirements
This pull request adds an informative error to explain why new parameters are now required to be in the turbine input file.
Think it's mostly ready except on my computer print out of message is oddly spaced
Impacted areas of the software
farm.py
Additional supporting information
Test results, if applicable
Can test by deleting either ref_density_cp_ct or ref_tilt_cp_ct from floris/turbine_library/nrel_5MW.yaml and running example examples/01_opening_floris_computing_power.py