Very minor clean-up and improvements

[yoelcortes]

@CalebBell,

As I was going through the phases submodule, I found a couple of places where I could make some minor improvements:

• Make T, P, zs properties
• Remove CEOSPhase.force_phase attribute (I could not find anywhere where it is used; it seems like the is_gas and is_liquid is used instead)
• Specify no additional slots for CEOSPhase subclasses to avoid python from adding dict when parent class slots is specified.

Please let me know if there is any issue with any of these and I can undo them. These are all very minor, but I'll slowly get into the groove into contributing to this library again. For more significant contributions, I would also consult with you beforehand to make sure the direction is OK.

I have a couple of other places I would like to make moderate changes, but I'll post them as a separate issue in case you have any comments before I proceed with a pull request.

Thank you!

[CalebBell]

Hello Yoel! Thank you for working to improve Thermo. I do have some comments about this PR.

• The force_phase attribute of CEOSPhase is used to ensure that a CEOSPhase object initiated with IGMIX as its eos is always a gas. I guess there aren't any tests for this and the default phase identification method gets it right 100% of the time, but it's conceivable enough for me that without this attribute a future development would make it so an ideal gas gets identified as a liquid. Using this flag also skips the phase check for that phase and I am more comfortable with this attribute remaining.
• The slots thing was a great catch. In general it is probably a lost cause to prevent phases from generating a dict attribute, and I am only trying to use it for speeding up attribute access. The dict is forced to be present on line 149 of phase.py. As there are hundreds of attributes that can be set to the phase object, it is better to keep many but not all dynamic and associated with a dictionary. I'm appreciative of the diligence though!
• I'm not sure how I feel about the change of T, P and zs to be properties. On the one hand it does save 24 bytes of ram per object; on the other it's another indirect reference and method call. Those attributes are accessed all over the place in the phase object, and I guess I really don't think it's worth saving the RAM in this case. The change can seem elegant but has a hidden performance hit in a very hot place. As it is, the actual T, P, and zs object values are shared in RAM.

The rest of the cleanup looks great! Thank you.

Sincerely, Caleb

[yoelcortes]

Thanks for the quick and detailed reply! Great points and I am happy to undo those changes. Here are a couple of followup comments:

• Thanks for your comment on __slots__! I did not realize you also had __dict__ in Phase.__slots__.
• I think the main benefit of the T, P, zs properties is to reduce code by not having to set T, P, and zs (which happens in __init__ and to methods). It would also speed up a few places (e.g., in flash loops which use to(T=T, P=P, zs=zs)). But I do agree, TPzs are accessed all over the place and using properties would probably be slower overall. I will keep in mind a general preference in thermo for making data accessibility faster over code reduction.

I do not completely follow your comment on force_phase. I hope you don't mind me asking for clarification.

• I cannot find where the force_phase skips the phase check. It looks like the composition_independent, is_gas, and is_liquid attributes serves a similar purpose in the EOS, Phase, and Flash classes. Did you mean the force_phase attribute does not do anything yet, but you have plans to use it in the future?

Thank you!

[CalebBell]

Hello Yoel, Of course I don't mind you asking for more information. The force_phase setting can skip a phase ID check for a phase in a multicomponent system here: https://github.com/CalebBell/thermo/blob/master/thermo/phase_identification.py#L800

In general, I think the ability to force a thermodynamic model to be recognized in thermo as a gas, liquid, or solid - always - is something that is desirable.

There are definitely a lot of places I've gone for speed over code reduction, and so far that's been OK. Other times where I went for speed and code reduction at the same time I've ran into trouble - for example issue https://github.com/CalebBell/thermo/issues/114 where it turns out doing string replacements on source code using the inspect module and exec to avoid duplication doesn't work for some workflows.

Sincerely, Caleb

[yoelcortes]

@CalebBell,

I reversed the changes we talked about and changed the scalar attribute to vectorized. I didn't realized this attribute was used in so many modules until I started making the changes, but I went ahead and completed it anyway. All tests passing. Let me know if there is anything else to complete for this pull.

Thanks!

[CalebBell]

This looks really great Yoel! Thank you. I hope things are well.