ulissigroup / GASpy

GNU Lesser General Public License v3.0
64 stars 33 forks source link

Function (& its test) that calculates electrochemical stability, updated version. #94

Closed apalizha closed 4 years ago

apalizha commented 4 years ago

Add a function that calculates electrochemical stability of bulk composition under reaction conditions. It is created as a stand-alone function because each chemical reaction has its own reaction condition, so it's better to have this function as a helper function.

ktran9891 commented 4 years ago

Just found this again. I remember @samueldy having some suggestions. What's the status on this PR?

apalizha commented 4 years ago

Made changes with all suggestions and this was the updated PR. Was waiting on it to be merged.

On Thu, Jun 11, 2020, 3:25 PM Kevin Tran notifications@github.com wrote:

Just found this again. I remember @samueldy https://github.com/samueldy having some suggestions. What's the status on this PR?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ulissigroup/GASpy/pull/94#issuecomment-642882685, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIDWL2NYD44V6OKRF5SQCQTRWEVRPANCNFSM4NEIKAXQ .

apalizha commented 4 years ago

@ktran9891 Think you could merge this when you get a chance? Thanks!

ktran9891 commented 4 years ago

Half of the unit tests across the board are failing now for various reasons. Might have something to do with pymatgen version control issues, test caching issues, and/or something else entirely. You said that all your tests are passing? Can you confirm that the test caches are updated and also confirm what image you are using when testing?

ktran9891 commented 4 years ago

Failing a test on my end:

gasdb_test.py::test_get_electrochemical_stability FAILED                                                                           [100%]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

    def test_get_electrochemical_stability():
        # at pH=0, V=0.9
        expected_stabilities = {'mp-126': 0.884,  # Pt
                                'mp-81': 0.0}    # Au
        for mpid, expected_stability in expected_stabilities.items():
            stability = get_electrochemical_stability(mpid, 0, 0.9)
>           assert math.isclose(stability, expected_stability, rel_tol=1e-6)
E           assert False
E            +  where False = <built-in function isclose>(0.861, 0.884, rel_tol=1e-06)
E            +    where <built-in function isclose> = math.isclose

gasdb_test.py:518: AssertionError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB post_mortem (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /home/GASpy/gaspy/tests/gasdb_test.py(518)test_get_electrochemical_stability()
-> assert math.isclose(stability, expected_stability, rel_tol=1e-6)
(Pdb) l
513         # at pH=0, V=0.9
514         expected_stabilities = {'mp-126': 0.884,  # Pt
515                                 'mp-81': 0.0}    # Au
516         for mpid, expected_stability in expected_stabilities.items():
517             stability = get_electrochemical_stability(mpid, 0, 0.9)
518  ->         assert math.isclose(stability, expected_stability, rel_tol=1e-6)
[EOF]
(Pdb) stability
0.861
(Pdb) expected_stability
0.884
apalizha commented 4 years ago

Yeah saw that too. Might be related to atoms_operator_test -->pmg stuff. Nothing was failing when I tested it. I'll look at it again this weekend altogether.

On Thu, Jun 25, 2020, 3:23 PM Kevin Tran notifications@github.com wrote:

Failing a test on my end:

gasdb_test.py::test_get_electrochemical_stability FAILED [100%]

traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

def test_get_electrochemical_stability():
    # at pH=0, V=0.9
    expected_stabilities = {'mp-126': 0.884,  # Pt
                            'mp-81': 0.0}    # Au
    for mpid, expected_stability in expected_stabilities.items():
        stability = get_electrochemical_stability(mpid, 0, 0.9)
      assert math.isclose(stability, expected_stability, rel_tol=1e-6)

E assert False E + where False = (0.861, 0.884, rel_tol=1e-06) E + where = math.isclose

gasdb_test.py:518: AssertionError

entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

PDB post_mortem (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> /home/GASpy/gaspy/tests/gasdb_test.py(518)test_get_electrochemical_stability() -> assert math.isclose(stability, expected_stability, rel_tol=1e-6) (Pdb) l 513 # at pH=0, V=0.9 514 expected_stabilities = {'mp-126': 0.884, # Pt 515 'mp-81': 0.0} # Au 516 for mpid, expected_stability in expected_stabilities.items(): 517 stability = get_electrochemical_stability(mpid, 0, 0.9) 518 -> assert math.isclose(stability, expected_stability, rel_tol=1e-6) [EOF] (Pdb) stability 0.861 (Pdb) expected_stability 0.884

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ulissigroup/GASpy/pull/94#issuecomment-649771113, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIDWL2LKR7PJNPFN2FUYI7DRYOPZLANCNFSM4NEIKAXQ .

apalizha commented 4 years ago

ehh... messed up couple things, had to discard some stuff, will open another PR.