Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Failing Joback Method Unit Test #2682

Merged
merged 2 commits into from
Jun 14, 2024
Merged

Fix Failing Joback Method Unit Test #2682

merged 2 commits into from
Jun 14, 2024

Conversation

JacksonBurns
Copy link
Contributor

#2680 and #2681 both fail this unit test despite not touching the associate code in any way:

            if comment:
>               assert transport_data.comment == comment
E               AssertionError: assert 'Epsilon & sigma estimated with Tc=500.53 K, Pc=48.02 bar (from Joback method)' == 'Epsilon & sigma estimated with Tc=500.53 K, Pc=47.11 bar (from Joback method)'
E                 - Epsilon & sigma estimated with Tc=500.53 K, Pc=47.11 bar (from Joback method)
E                 ?                                                 ^ ^^
E                 + Epsilon & sigma estimated with Tc=500.53 K, Pc=48.02 bar (from Joback method)
E                 ?                                                 ^ ^^

test/rmgpy/data/transportTest.py:181: AssertionError

...I think because we updated the Joback parameters in RMG-database here: ReactionMechanismGenerator/RMG-database#636

I've added a dummy commit just to trigger the CI and check that this fails again, and then I intend to update the expected values to what we are getting now.

cc @jonwzheng TL;DR: the Joback coefficient changes we made in RMG-database are failing a unit test on RMG-Py, this PR will fix it

@jonwzheng
Copy link
Contributor

whoops! looks like we did need to consider the unit tests a bit more ...

from reading the code looks like we expect it to be 47.11 but it should be 48.02 (with our new code)

I will also double check against JRgui to see what that implementation of the joback parameters returns.

@jonwzheng
Copy link
Contributor

JRgui agrees that Pc of acetone should be 48.02 bar. We should just amend the unit test comment to be 48.02, i.e.:

        self.testCases = [
            [
                "acetone",
                "CC(=O)C",
                Length(5.36421, "angstroms"),
                Energy(3.20446, "kJ/mol"),
                "Epsilon & sigma estimated with Tc=500.53 K, Pc=48.02 bar (from Joback method)",
            ],

JacksonBurns added a commit that referenced this pull request Jun 14, 2024
@jonwzheng
Copy link
Contributor

Looks like the transport properties are good to go now, but any idea about the CI failure @JacksonBurns? Looks like there's an issue with the GH actions artifact download, which is odd to me as it wasn't showing up before.

@JacksonBurns
Copy link
Contributor Author

Ah this is vexing, the CI is trying to download the stable_regression_results from the last run of the CI on the main branch, which was the scheduled run last night. Unforunately that run failed because of the unit tests that are being fixed in this PR...

@jonwzheng I think we just need to bypass merge this PR. Even if it does introduce regression test changes, we know that they are 'correct' - the equation that we fixed was unambiguously wrong.

@JacksonBurns JacksonBurns marked this pull request as ready for review June 14, 2024 17:39
@jonwzheng
Copy link
Contributor

Okay sounds good. Pleas remove the dummy commit ("DROPME") and I'll go ahead and approve.

@JacksonBurns
Copy link
Contributor Author

Done! Swag removed

Copy link
Contributor

@jonwzheng jonwzheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (noting that CI fail in this is okay. Unit tests have been fixed.)

@jonwzheng jonwzheng merged commit 3454d0b into main Jun 14, 2024
4 of 6 checks passed
@jonwzheng jonwzheng deleted the fix/Pc_joback_updates branch June 14, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants