User methods (for pure chemicals)

[yoelcortes]

Hi Caleb,

I would like to propose a new API for setting user methods. I do realize that one is already in place (https://thermo.readthedocs.io/property_objects.html#adding-new-methods), but it's still incomplete and untested enough that I think a new implementation may work better.

I'll first note a few minor issues with the current implementation for user or "local" methods:

• It allows users to add methods multiple times, but I don't think anyone is actually dependent on this feature because it has a bug that doesn't let it run twice. Perhaps this feature made sense when thermo would loop through methods to find one that works, but with extrapolation implemented, just one method should be enough. From my own experience with biosteam users, no user has ever tried to add multiple "methods".
• The derivative and integration functions passed by the user are not actually used (code still incomplete).
• add_method is not tested in CI (I checked the coverage).
• Having a tuple of all the functions would make it a bit hard to read and write (maybe an object with slots would be better?).

With this pull request, I propose a "set_user_method" method which allows for only one user method. All the derivatives and integrals are working as they should (tests have been added with full coverage and all tests passing). I made this function with biosteam users in mind, so there are a few convenience features you might take notice of. It would be straight forward to add a similar "add_user_method_P" method for pressure-dependent methods, but this is not too important for me yet.

This pull request draft is not urgent, so feel free come back to this later if you need to think about the implementation more.

Thanks again!

[CalebBell]

Hi Yoel! Thanks for testing out this functionality! I only added it a few months ago and I didn't test it too much. One of the issues I found with it was that I couldn't easily store to disk property objects once a function-style method had been added. Have you looked at the this section of the documentation? https://thermo.readthedocs.io/property_objects.html#adding-new-correlation-coefficient-methods One thing about coefficient-based methods is that they can transition to using numbs-accelerated methods, if the module is imported from thermo.numba.

Using a tuple to store those was a mistake. I didn't think too hard about it because it was an internal detail though, so it's easy to fix. I would have sworn I used this with multiple different methods at a time, though! I think there really are a lot of use cases for the functionality of adding multiple different function-style methods. As an example, in this sample three methods are added - one for each phase of solid oxygen: https://thermo.readthedocs.io/property_objects.html#adding-new-correlation-coefficient-methods-from-data

I would like to keep the name and functionality of multiple user-input functions. I also don't think it's fair to say the method was untested - it has some tests in the documentation https://thermo.readthedocs.io/property_objects.html#adding-new-methods Unfortunately those aren't ran automatically because too many of the Chemical and Mixture doctests are failing. Fortunately, writing this now reminds me that I can probably configure pytest to skip those files. That should improve test coverage nicely. Thank you again for introducing me to the NUMBERS flag in pytest. Doctests are just so great and a total breath of fresh air compared to big complex tests in other programming languages!

Adding a variant for pressure-dependency also seems like a great addition! I've been making a big push on temperature-dependent data recently, but not pressure dependent as well yet.

[yoelcortes]

Thanks for the quick and detailed reply! I haven't thought about the multiple solid phases... so adding multiple methods with "add_method" makes good sense.

I'll can take care of addressing the last 3 issues I noted in my previous post without changing the functionality of "add_method".

Would you also like to keep the "f_der_general" parameter in "add_method"? It isn't currently used for anything, and I'm not sure if it is worth writing additional code to implement.

Thanks again!

[CalebBell]

Hi Yoel! There is a thing called rubber-ducky debugging, where you explain a code problem to an actual or imaginary rubber ducky as if they are a person. It can help think about problems from someone else's perspective. Thinking is hard and as humans we aren't always good at it. Changing your perspective is often most of the way to a solution.

Just seeing your perspective about f_der_general was enough to convince me no one will ever use it, so yes please let's just take it out.

This work is greatly appreciated!

[yoelcortes]

Done! To summarize, this pull request makes the following additions:

• Deprecate f_der_general parameter.
• Introduce new "LocalMethod" class for storing functions that define a local method.
• Introduce "ConstantLocalMethod" for user convenience (let me know if you have any comments/concerns on this).
• Make use of f_der1, f_der2, f_der4, f_int, and f_int_over_T functions in calculate_derivative, calculate_integral, and calculate_integral_over_T methods.
• Add unit tests for local methods.
• Make Tmin, Tmax, and name optional (hope this is fine with you, but feel free to make these non-optional).

A "add_method_P" may be needed in the future, but I think it can be implement by demand.

Please feel free to make backward changes as you see fit. No need to make a new release just yet, but it would be nice to merge soon.

Happy to make contributions, your work on thermo is much appreciated too! Thanks,

[CalebBell]

Looks awesome!

[CalebBell]

Merged!