Grid-class tidying #751
Grid-class tidying #751
Conversation
|
Looking good so far @rafmudaf. Let me know if you'd like help on this anywhere; otherwise I'll keep an eye on it and review fully when you're ready. |
|
Yes @rafmudaf this seems like a very helpful simplification, thank you and please let me know if I can help! |
rafmudaf
force-pushed
the
grid_cleanup
branch
2 times, most recently
from
1a5d06a
to
6155830
Compare
Initially, this variable held a single reference turbine diameter value, but it was subsequently changed to contain the diameter of all wind turbines. The name change simply reflects it’s actual use.
Rather than a list of Vec3. This supports creating a layout from a dictionary of exported modules and classes rather than having to do the additional step of converting to Vec3.
This requires setting the comparison method in the TurbineGrid and Grid classes. While this can be done with field(eq=cmp_using(eq=np.array_equal)) this adds a lot of additional code to the attribute declarations and makes them difficult to read. Holding off for now but I wanted to leave the test in place for future reference.
After switching the Grid coordinates from Vec3 to Numpy arrays, it turned out this wasn’t really used anymore.
While this was useful early in the v3 redesign, we’ve converged to a design where this is no longer used.
rafmudaf
force-pushed
the
grid_cleanup
branch
from
6155830
to
f90108e
Compare
rafmudaf
added
enhancement
| @@ -36,51 +36,6 @@ | |||
| from floris.utilities import cosd | |||
|
|
|||
|
|
|||
| def _filter_convert( | |||
There was a problem hiding this comment.
I didn't initially follow why this was removed, but realized that the way it's invoked makes it unnecessary.
There was a problem hiding this comment.
I should have made a better note about this than vaguely saying I removed it because it wasn't used. Ultimately, the solver is the only place where the ix_filter is use and it's always a Python list which is the one case where the _filter_convert function isn't needed.
I think I was going a bit heavy on deleting things when I removed this one, so I'm happy to add it back. The motivation to delete unused code is to have a less complex starting point for the v4 work.
There was a problem hiding this comment.
No, I think this function outlived its usefulness, so I'm fine with removing it. It made sense earlier on when we assumed others might be directly invoking some of the turbine calculations, but that's not how it panned out over time.
Clean up the Grid classes and support from_dict, as_dict
Because of the dependency on
Vec3as an input type, the Grid classes currently do not support instantiation from a dictionary and exporting to a dictionary. This is not currently an issue because these data types are only created from withfloris.simulationand are not exposed to the user, but this does complicate the testability of these classes. This pull request addresses this by using Numpy arrays for the Grid coordinates rather than Vec3's. After making the change above, I realized that Vec3, in general, is not very much used. It was quite easy to remove completely, so that's also included here.Additionally, I've renamed some files and variables to better reflect their use and scope, and I fixed an inconsistency in the dimensionality of the unit tests fixtures. Other "tidying" is included such as removing the unused
_filter_convertfunction.Related issue
Depends on #750
Additional supporting information
Given significant upcoming changes to FLORIS (see the v4 project), this change allows us to have a better understanding of the architecture of the existing code so that we can more easily and accurately build upon it.