Migrate Pourbaix workflow to pyEQL v1.6.1

[SuixiongTay]

Description

This PR migrates the previous pourbaix to pourbaix_pyEQL_v1.5 branch.

Content

1. Core modules: Core pourbaix workflow and diagram construction

• pourbaix_api.py
• pourbaix_diagram.py
• compatibility.py

3. Databases: Reference databases and correction schemes

• NBS_Tables_Library.xlsx
• MP2020Compatibility.yaml
• MITCompatibility.yaml
• mpr_reference_ion_database.json
• pourbaix_test_data.json

4. Tests: Test coverage from pymatgen

• test_pourbaix_api.py
• test_pourbaix_diagram.py
• test_compatibility.py

5. Tutorials

• pyeql_tutorial_pourbaix_Ca-Mg-C.ipynb
• pyeql_tutorial_pourbaix_Na-K-Mg-Cl.ipynb

Note

• emmet-core uses typing.NotRequired which was introduced in python 3.11.
• Therefore, pytest.importskip is used to skip through python 3.10 to avoid the error.

Addressing #368

[SuixiongTay]

Hi @rkingsbury , I noticed the pourbaix branch is quite old and there were a lot of commits since then.

Should I target this PR against the pourbaix branch, or would you prefer this PR against the current main and let you assign the branching? Thanks!

[codecov]

Codecov Report

❌ Patch coverage is 75.44910% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.17%. Comparing base (dbdfbce) to head (77e9098).
⚠️ Report is 892 commits behind head on pourbaix.

Files with missing lines	Patch %	Lines
src/pyEQL/pourbaix/pourbaix_api.py	74.16%	26 Missing and 5 partials ⚠️
src/pyEQL/pourbaix/compatibility.py	83.87%	3 Missing and 2 partials ⚠️
src/pyEQL/pourbaix/pourbaix_diagram.py	68.75%	5 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           pourbaix     #369       +/-   ##
=============================================
+ Coverage     71.75%   87.17%   +15.42%     
=============================================
  Files            13       17        +4     
  Lines          2687     3112      +425     
  Branches        481      549       +68     
=============================================
+ Hits           1928     2713      +785     
+ Misses          676      290      -386     
- Partials         83      109       +26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rkingsbury]

Hi @SuixiongTay , sorry I somehow missed your comment from 4/1. I've rebased your changes in the pourbaix branch to apply on top of main as of today. I'll do it again after #403 is merged, which may have some impact on your code.

[SuixiongTay]

Hi @rkingsbury, quick question: Regarding rebasing onto pourbaix branch, my understanding is that it would bring in the previous commits from pourbaix branch into this branch. Is that correct? Or should we directly compare this PR against the pourbaix branch instead of main?

[rkingsbury]

Good question - for this PR, please compare against pourbaix rather than main (I think you can change the target branch, if not, I can). That way the diff will show just your changes on top of the previous addition of pourbaix code.

[SuixiongTay]

Thanks @rkingsbury! I dont seem to have the option to edit the target branch. Would you mind helping me change it?

[rkingsbury]

Thanks @rkingsbury! I dont seem to have the option to edit the target branch. Would you mind helping me change it?

Done. You'll need to reconcile some merge conflicts it looks like. Lmk if you get stuck!

[SuixiongTay]

Hi @rkingsbury , this PR is ready for your review. I have also included two examples: pyeql_tutorial_pourbaix_Ca-Mg-C.ipynb and pyeql_tutorial_pourbaix_Na-K-Mg-Cl.ipynb which are representative of the Pourbaix diagrams in the manuscript.

[rkingsbury]

Thanks for this @SuixiongTay ! This really helps me understand your methods and changes. Overall looks good. I've made a series of small comments, including some opportunities for refactoring / streamlining that we can tackle separately or at another time.

The biggest / most important things to address before merging this are:

1. I don't think it's necessary (or advisable) for us to carry any of the solid compatibility classes in pyEQL. All we need to care about is the AqueousCompatibility. We can import all the **Compatibility classes (MP, MP2020, MIT) and associated .yaml files from pymatgen if we need them. Making this change will simplify this PR quite a bit by eliminating some extra files.
2. Make sure you have added or updated docstrings and references whereever appropriate to be consistent with your changes. (See specific comments). It's easy to copy/paste stuff, forget to update the docstring or reference, and then have a hard time figuring out where a number came from later on.
3. Clarification: (why) do we need a separate method modified_get_ion_entries_in_chemsys? I understand it might have made sense to develop it side-by-side initially, but is there any reason not to just merge the changes into the regular get_ion_entries_in_chemsys?

[rkingsbury]

Please delete the checkpoint file (and/or edit .gitignore to correctly ignore them - this is actually a problem right now.

(btw, the checkpoint file exposed your MP API Key, so you might want to re-generate it)

[rkingsbury]

Both notebooks look good! In this one, please add a short explanatory comment near the examples to explain why, in Example 1, you set comp_dict and conc_dict equal to one another, but in Examples 2 and 3, you set comp_dict to correspond to the target phase of interest.

[rkingsbury]

This is just a note for the future given the volume of expanded data here it makes most sense to store all the entropies separately in a .json or .yaml file and read it in here.

Can you add a TODO comment in the code about this? (or just do it now, if you prefer)

[rkingsbury]

Update this comment

1. molecular-like compounds -> pure elements, molecular gases, and reference solids
2. Add or update the reference - I think most of your entropies come from NIST rather than Kumaschewski, right?

[rkingsbury]

Add descriptions for these new kwargs to the docstring (Args: section). Remember to include units!

[rkingsbury]

This should import from pyEQL's version

[rkingsbury]

Should import the pyEQL version

[rkingsbury]

should import the pyEQL version

[rkingsbury]

Please revert this change to whatever the upstream branch has, unless this bump is required to make your changes work (this requirement will be updated separately elsewhere)

[rkingsbury]

Pare this file down to ONLY test MPAqueousCompatibility - in keeping with my bigger comment to remove all the **Compatibility classes except the aqueous one from pyEQL.