Minor enhancements to T and TPDependentProperty objects

[yoelcortes]

Hi Caleb,

This pull request is meant to clean-up and speed up T and TPDependentProperty objects. Main changes include:

• Remove unused attributes (probably from version 0.1), including sorted_valid_methods, which is an empty list that offers no functionality.
• Remove double documented attributes, such as "all_methods".
• Remove class attributes such as "_method", which are always initialized in __init__.
• Speed up valid_methods and valid_methods_P (made algorithm changes that do not affect the end result).
• Prevent loading extrapolation coefficients during initialization (and attempting to reload each time a method is set). Now coefficients are only loaded as needed.

The above changes speed up initialization of some T and TPDependentProperty objects by about 10 us (or 20% of the total creation time). The actual time varies depending on the chemical and the subclass of course.

These are minor changes (and all tests are passing), but just wanted to run this by you just in case I missed something.

Thanks!

[CalebBell]

Hello Yoel, It is great to hear from you! I hope all is well. These are a really nice set of clean-ups, thank you very much! I really like the valid_methods fixes. Code like that often becomes ugly from a series of independent changes over the years, so your re-write is great! I am seeing much more than a 20% speed boost in the random instances I am testing - more like 50%! I had intended to get around to lazy-loading the extrapolation coefficients as well. Depending on the method it can take quite a bit of time, for something that may never be used! I like how clean your code solution is and I am very grateful to have it in Thermo. One aspect of the TDependentProperty class I am ashamed of is how many function calls are required to calculate a property. I did a review, and found one unnecessary call to "method" I could clean up as well. With your changes now, the call tree to within-range functions is:

• T_dependent_property
• calculate
• (actual calculation function usually in chemicals)
• test_property_validity

For extrapolation, the tree is

• T_dependent_property
• extrapolate
• _extrapolate
• (actual extrapolation function call)

The two method that might be "removable" in that list is test_property_validity and _extrapolate. Removing test_property_validity would happen by validating all the calculation methods, to ensure they return None if something very wrong happens. This is probably possible but will be a fair bit of work. I could see myself adding a little duplication of code to remove the "_extrapolate" cleanup method you introduced. Your way is definitely cleaner; but this is a "hot" path that will be hit a lot, and function calls are very expensive.

Today, this isn't important to me though and I am very happy for your improvements! Are you looking for a new release with these ASAP or did you just do all this work out of the goodness of your heart?

Sincerely, Caleb

[yoelcortes]

@CalebBell, glad that the changes were good overall!

Yeah, agreed about the additional function call with "_extrapolate", need to keep "hot" paths as fast as possible... The "_extrapolate" implementation was my attempt to make simplifying changes without having to touch the code too much. I just pushed some new commits that removes the function call to "_extrapolate" by storing all extrapolation coefficients in a "cache" dictionary. All tests are passing, feel free to merge whenever.

I would be happy if we could remove the need for a test_property_validity method, but as you noted, it may not be as simple as it sounds... especially for complex functions as the "validity" might depend on the input parameters. A function that calculates Tmin, Tmax, Pmin, Pmax from the input paremeters may be needed for each function in the chemicals library. For now, I don't mind if we keep the test_property_validity indefinitely for robustness sake.

Thanks for offering to make a new release, but we can postpone it for a bit. I'm working on a new "1.0alpha" version of thermosteam which can load the T and TPDependentProperty objects instead of the current "model handles". I'd like to use thermo as much as possible; it's a powerful and mature software with an experienced and active lead developer. We would also get less overlap and more collaboration. So yeah, let's say the work is out of the goodness of my heart jaja

Thanks!