KingsburyLab / pyEQL

A Python library for solution chemistry
Other
65 stars 17 forks source link

Replace autogenerate() with a Solution class method #67

Closed kirill-push closed 10 months ago

kirill-push commented 11 months ago

Summary

Issue #63

Major changes:

Todos

If this is work in progress, what else needs to be done?

Checklist

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install
codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Files Coverage Δ
src/pyEQL/functions.py 16.00% <100.00%> (+1.08%) :arrow_up:
src/pyEQL/solution.py 83.85% <100.00%> (+0.72%) :arrow_up:

:loudspeaker: Thoughts on this report? Let us know!

kirill-push commented 10 months ago

Hi @rkingsbury! I have made changes, except for those for which I had a question for you above. Can you please take a look?

Just in case, I'll copy this question here:

Thank you for your comment @rkingsbury ! Sorry for my long execution of this task. The last few days I've been trying to get it to work with a single line loadfn(filename), but I still have some difficulties.

Are you sure that with your complex class Solution everything should work with a single line return loadfn(file name)? Because inside the class Solution you have, for example, Quantity elements, and for this yaml needs a custom representator and a custom constructor, if I'm not mistaken. Also for example for the case s_old = Solution(...)if we save it and then load from yaml s_new=loadfn(filename) than if we print(s_new) then we will see that s_new has pH: 0.0, but for s_old it is for example pH: 7.0. This is because the self.str method calls the self.pH() method, which calls the self.p() method, which calls self.get_amount('H+', "mol/L").magnitude, which gives 0, because the standard yaml constructor creates self.components not as a FormulaDict, but as a regular dict, and self.components doesn't store key H+, but stores H[+1] and for standart dict it causes KeyError and self.get_amount returns 0. And there are also examples of difficulties.

At this moment I am able to make both dumpfn and loadfn in the case of yaml work similar to the case of json, but it only looks like it, because when I check the instance of the class after loadfn in detail, I discover new bugs.

If you check the code monty.dumpfn and monty.loadfn (link to repo), you will see that they do not have the same encoder and decoder for yaml as they have for json. I also see that you use their base class MSONable to dump and load json, which apparently also helps the functions work in json mode, since it has basic methods to_json and from_json.

If you need, I can continue trying to do it in one line, as is the case with json, but it will take time and additional methods to read and write yaml files correctly.

rkingsbury commented 10 months ago

Hi @rkingsbury! I have made changes, except for those for which I had a question for you above. Can you please take a look?

Just in case, I'll copy this question here:

Thank you for your comment @rkingsbury ! Sorry for my long execution of this task. The last few days I've been trying to get it to work with a single line loadfn(filename), but I still have some difficulties. Are you sure that with your complex class Solution everything should work with a single line return loadfn(file name)? Because inside the class Solution you have, for example, Quantity elements, and for this yaml needs a custom representator and a custom constructor, if I'm not mistaken. Also for example for the case s_old = Solution(...)if we save it and then load from yaml s_new=loadfn(filename) than if we print(s_new) then we will see that s_new has pH: 0.0, but for s_old it is for example pH: 7.0. This is because the self.str method calls the self.pH() method, which calls the self.p() method, which calls self.get_amount('H+', "mol/L").magnitude, which gives 0, because the standard yaml constructor creates self.components not as a FormulaDict, but as a regular dict, and self.components doesn't store key H+, but stores H[+1] and for standart dict it causes KeyError and self.get_amount returns 0. And there are also examples of difficulties. At this moment I am able to make both dumpfn and loadfn in the case of yaml work similar to the case of json, but it only looks like it, because when I check the instance of the class after loadfn in detail, I discover new bugs. If you check the code monty.dumpfn and monty.loadfn (link to repo), you will see that they do not have the same encoder and decoder for yaml as they have for json. I also see that you use their base class MSONable to dump and load json, which apparently also helps the functions work in json mode, since it has basic methods to_json and from_json. If you need, I can continue trying to do it in one line, as is the case with json, but it will take time and additional methods to read and write yaml files correctly.

Hi @kirill-push , thanks so much for investigating. This is all a surprise to me - my understanding was the MSONable and as_dict / from_dict create a serializable dict representation that can be fully reconstituted by loadfn, regardless of format. But I'll admit I typically have used .json with loadfn and dumpfn so I might just be mistaken.

In any case, those are issues that we do not need to sort out in this PR. Overall this looks great. I'm going to request some minor edits just as finishing touches and then I'm ready to merge!

rkingsbury commented 10 months ago

Thanks very much for this contribution @kirill-push !