Feature/add ref density

[paulf81]

It is mostly ready to be merged, if we agree that it should be merged

Tests don't yet pass either so setting this to draft for now and setting milestone to 3.2 to give us some time to think if this is the right way to do it

Feature or improvement description
This pull request responds to the discussion #391 and the request for specifying a reference air density at which the provided Cp/Ct curves are given in the event this is not 1.225

Questions I have:

1. Does this slow down the code at all?
2. Does this pose any back compatibility, should we supply a default if this term not included?

Related issue, if one exists
Discussion #391

[ghost]

It looks great! I've done some testing and haven't found any issues (but I am not an expert!). I hope that the performance of the code is not an obstacle. Thank you very much :)

[paulf81]

@bayc and @rafmudaf I think this is now fixed in that the tests pass, I'd like your opinion on the change I made to type_dec.py, it already through an error if the new ref density is not defined, but since this is happening on purpose I thought it good to give, maybe temporarily, a "friendlier", more informative warning, what do you think?

[paulf81]

@rafmudaf and @bayc I think I'm having second thoughts, I had set the code if your turbine YAML doesn't specify the air density the cp/ct are defined at. But maybe more consistent would be to warn that since you didn't specify we will assume it is 1.225?

[paulf81]

@rafmudaf and @bayc I might need a little help from you finishing this up. I just removed the error on not including ref_air_density in favor instead applying a default value. But I can't seem to figure out where the default value code should go. I can't see where the variable is assigned within the Turbine base class so that I could make an intercepted default assignment and give a warning. If I remove the value from the input turbine yaml file: nrel_5MW.yaml the error for the missing value is thrown by the function from_dict within type_dec.py so this was my first guess but @bayc was thinking (and I agree) this is too generic a spot to muddle with a specific default catch. Any suggestions for me? Thanks!!

[rafmudaf]

Great question! This is worth writing about some more in a developer's guide, so I've made note of that here.

Since we use the attrs library to create most of the data structures in floris.simulation, you can use an optional argument to field() to supply a default value. Try something like this:

ref_density_cp_ct: float = field(converter=float, default=1.225)

[paulf81]

ok @rafmudaf I was working on this approach but I think there's then a second problem, the code in farm.py around building up self.turbine_definitions (lines 80-87) uses a direct read of the YAML file (it doesn't refer back to the turbine object) such that when this code

    def construc_turbine_ref_density_cp_cts(self):
        self.ref_density_cp_cts = np.array([turb['ref_density_cp_ct'] for turb in self.turbine_definitions])

(line 110)

looks for the value it's not there since the default is only set within the turbine class, should I make also a second setting of the default also for turbine_definitions?

[rafmudaf]

@bayc are the properties in the construct_* routines intentionally not using the turbine_map? Some are and some aren't, so I'm wondering if there was a specific reason for it.
See https://github.com/paulf81/floris/blob/feature/add_ref_density/floris/simulation/farm.py#L107 vs https://github.com/paulf81/floris/blob/feature/add_ref_density/floris/simulation/farm.py#L116.

[paulf81]

Talking this over with @bayc I think I'll go back to not giving a default and making an error. Having the default/warning leads to a lot checks required at different places in the code. Also, I was thinking a lot of people would get an error, but I think I was thinking of main input yaml. At least we'll push up the changes to the turbines in the turbine_library

[paulf81]

Nevermind! I think I found a way to do it... one sec...

[paulf81]

ok @bayc and @rafmudaf I think this works pretty well now, if you'd like to test, just run example 1 with the ref_air_density deleted out of nrel_5MW.yaml

[rafmudaf]

@paulf81 I've made a discussion (#488) to keep track of things that we mark as "will be changed in a future release". This is for the benefit of users and developers. Users can know what will change and the NREL team will have a checklist to review prior to future releases to make sure we did the thing that was noted.

In what release do you think that input entry would be made required? Want to say v3.3?

[paulf81]

I think v3.3 sounds good

@rafmudaf I can't seem to figure out this error on the code check, it points to an issue (I think) in turbopark_regression_test.py, but the code it says is erroring, I don't see it, it's like it has an old version stuck in it or something? Can you tell what is going on?

I think after that this is ready to merge in my mind

[paulf81]

@bayc success! squash and merge time?