Skip to content

Improve attrs usage in simulation package#750

Merged
rafmudaf merged 11 commits intoNatLabRockies:developfrom
rafmudaf:attrs_fix
Dec 6, 2023
Merged

Improve attrs usage in simulation package#750
rafmudaf merged 11 commits intoNatLabRockies:developfrom
rafmudaf:attrs_fix

Conversation

@rafmudaf
Copy link
Collaborator

@rafmudaf rafmudaf commented Dec 1, 2023

Fix and improve attrs usage in floris.simulation package

Starting in v3, FLORIS adopted the attrs library as the method for creating the data structures throughout the floris.simulation package. There are numerous benefits of using attrs including that slotted classes are more memory efficient. However, this adopted of attrs as the infrastructure for created classes was incomplete. Specifically, BaseModel and BaseClass were not attrs-derived, and so their subclasses were a mix of slotted and dict-based classes. In addition to missing out on potential memory efficiency, this allowed for monkey patching which resulted in an inconsistent architecture within a few class definitions.

This pull request ensures the usage of attrs-derived classes throughout the floris.simulation package primarily by adopting attrs in the BaseClass and BaseModel definitions. In order to maintain a fully attrs-derived inheritance tree while avoiding attrs creeping into floris.tools, the use of the logging class was changed from inheritance to composition within floris.simulation and its use remains unchanged in floris.tools - except the name changed from LoggerBase to LoggingManager to reflect that it isn't necessarily a base-class. If it were to remain only a base-class, then it would have to use attrs to enable slotted classes. Lastly, a few monkey patched attributes were added to their corresponding class declarations.

Related issue

This is tangentially related to #699. I came across this while testing that one.

Impacted areas of the software

The infrastructure to create the classes within floris.simulation.

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.

These were monkey patched, but slotted classes do not allow monkey patching. See the previous commit for the change that enabled slotted classes.
This allows for the floris.simulation package to use attrs and slotted classes while the floris.tools package can remain typical Python classes.
The LoggerBase did have @define in order to maintain the slotted classes, but that is removed here.
@rafmudaf rafmudaf added enhancement An improvement of an existing feature floris.simulation labels Dec 1, 2023
@rafmudaf rafmudaf added this to the v3.6 milestone Dec 1, 2023
@rafmudaf rafmudaf self-assigned this Dec 1, 2023
@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Dec 1, 2023

@bayc One thing I noticed (via the failing examples) is that Farm.turbine_fCts_sorted is only created for the multidimensional Cp and Ct turbine feature. Is there any reason not to do this for all cases? We could make this part of the if-statement the default and the other is the special case.

@rafmudaf rafmudaf requested a review from RHammond2 December 1, 2023 03:12
@rafmudaf rafmudaf mentioned this pull request Dec 1, 2023
@paulf81
Copy link
Collaborator

paulf81 commented Dec 1, 2023

This sounds like a great catch and really useful improvement @rafmudaf , thank you!! Do you think we'll see a difference in memory usage after merging or is this more of a preventative measure against issues?

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Dec 1, 2023

There's no runtime issue that this fixes, but it does improve the readability of the code since you can now scan the attributes at the top of each class and be sure that those are all of the ones available. Before this change, anything can be tacked on to any object at any time.

As for the memory consumption, we don't have the infrastructure in place to measure this, so I think there will be an improvement but I don't have a method to prove it.

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Dec 1, 2023

I've added the missing attribute declarations (see #750 (comment) and #711), and I'll open a separate issue to document the current state of the Farm class architecture and how to make it consistent throughout.

Copy link
Collaborator

@RHammond2 RHammond2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is pretty good overall, but there are a couple of things I made comments on, that may or may not need to be addressed.

I assume by using the attrs slotted class more explicitly, there might be a speedup, but I wouldn't assume it's anything noteworthy.

Copy link
Collaborator

@RHammond2 RHammond2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the all comments I made are adequately addressed, so I'm switching to approved.

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Dec 6, 2023

Thanks for the thorough review @RHammond2

@rafmudaf rafmudaf merged commit 52b8a5f into NatLabRockies:develop Dec 6, 2023
@rafmudaf rafmudaf deleted the attrs_fix branch December 6, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement An improvement of an existing feature floris.simulation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants