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

Deprecate old Cubic EoS #1519

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dallan-keylogic
Copy link
Contributor

Summary/Motivation:

The old cubic equation of state has not been updated to have correct formulae for partial molar Gibbs and Helmholtz free energy, and doesn't have any formulae for heat capacity or speed of sound. Since it's been made obsolete by the cubic EoS implemented through the Modular Property Framework, there's no reason to keep it around to confuse people.

Changes proposed in this PR:

  • Deprecate old Cubic EoS property block, parameter block, and initializer.
  • Deprecate BT Peng-Robinson class based on old Cubic EoS

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.04%. Comparing base (3e403b7) to head (171fecf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1519      +/-   ##
==========================================
- Coverage   77.05%   77.04%   -0.01%     
==========================================
  Files         384      384              
  Lines       62335    62338       +3     
  Branches    10222    10222              
==========================================
  Hits        48031    48031              
- Misses      11875    11879       +4     
+ Partials     2429     2428       -1     

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

@dallan-keylogic dallan-keylogic marked this pull request as ready for review November 4, 2024 20:00
@dallan-keylogic dallan-keylogic self-assigned this Nov 4, 2024
@dallan-keylogic dallan-keylogic added Priority:High High Priority Issue or PR backward-compat Affects backward compatibility deprecation Add deprecation to codebase and/or remove deprecated feature labels Nov 4, 2024
Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

Looks OK to me - just to confirm, have you checked the examples to make sure there is nothing there that depends on this?

@dallan-keylogic
Copy link
Contributor Author

Looks OK to me - just to confirm, have you checked the examples to make sure there is nothing there that depends on this?

That's what the CI tests are for. Yes, I have checked, nothing in the examples uses CubicParameterBlock or BTParameterBlock.

Copy link
Contributor

@bpaul4 bpaul4 left a comment

Choose a reason for hiding this comment

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

Looks good, @dallan-keylogic.

@dallan-keylogic
Copy link
Contributor Author

Got a test failure on Python 3.10 Windows, completely unrelated to the PR.

================================== FAILURES ===================================
______________________________ test_parity_alamo ______________________________

alamo_surrogate = <idaes.core.surrogate.alamopy.AlamoSurrogate object at 0x0000020CEE61A7A0>
data_validation =     Bypass_Fraction  NG_Steam_Ratio  ...        N2            O2
0          0.171014        1.000000  ...  0.341778  2...4  ...  0.317834  3.530000e-20
19         0.262319        1.200000  ...  0.324708  3.670000e-20

[20 rows x 15 columns]

    @pytest.mark.unit
    def test_parity_alamo(alamo_surrogate, data_validation):
    
        with TempfileManager.new_context() as tf:
            # note - a failure 'The process cannot access the file because it is
            # being used by another process' could occur if an internal error
            # arises before the results file is closed inside the surrogate method
    
            # create and step into new temporary directory
            dname = tf.mkdtemp()
            filename = os.path.join(dname, "results.pdf")
>           figs = surrogate_parity(
                alamo_surrogate, data_validation, filename=filename, show=False
            )

idaes\core\surrogate\plotting\tests\test_alamo_plotting.py:117: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
idaes\core\surrogate\plotting\sm_plotter.py:206: in surrogate_parity
    return _parity(
idaes\core\surrogate\plotting\sm_plotter.py:249: in _parity
    fig = plt.figure()
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\matplotlib\pyplot.py:1027: in figure
    manager = new_figure_manager(
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\matplotlib\pyplot.py:550: in new_figure_manager
    return _get_backend_mod().new_figure_manager(*args, **kwargs)
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\matplotlib\backend_bases.py:3507: in new_figure_manager
    return cls.new_figure_manager_given_figure(num, fig)
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\matplotlib\backend_bases.py:3512: in new_figure_manager_given_figure
    return cls.FigureCanvas.new_manager(figure, num)
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\matplotlib\backend_bases.py:1797: in new_manager
    return cls.manager_class.create_with_canvas(cls, figure, num)
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\matplotlib\backends\_backend_tk.py:483: in create_with_canvas
    window = tk.Tk(className="matplotlib")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <tkinter.Tk object .>, screenName = None, baseName = 'pytest'
className = 'matplotlib', useTk = True, sync = False, use = None

    def __init__(self, screenName=None, baseName=None, className='Tk',
                 useTk=True, sync=False, use=None):
        """Return a new top level widget on screen SCREENNAME. A new Tcl interpreter will
        be created. BASENAME will be used for the identification of the profile file (see
        readprofile).
        It is constructed from sys.argv[0] without extensions if None is given. CLASSNAME
        is the name of the widget class."""
        self.master = None
        self.children = {}
        self._tkloaded = False
        # to avoid recursions in the getattr code in case of failure, we
        # ensure that self.tk is always _something_.
        self.tk = None
        if baseName is None:
            import os
            baseName = os.path.basename(sys.argv[0])
            baseName, ext = os.path.splitext(baseName)
            if ext not in ('.py', '.pyc'):
                baseName = baseName + ext
        interactive = False
>       self.tk = _tkinter.create(screenName, baseName, className, interactive, wantobjects, useTk, sync, use)
E       _tkinter.TclError: Can't find a usable init.tcl in the following directories: 
E           C:/Miniconda/envs/idaes-pse-dev/Library/lib/tcl8.6 C:/Miniconda/envs/lib/tcl8.6 C:/Miniconda/lib/tcl8.6 C:/Miniconda/envs/library C:/Miniconda/library C:/Miniconda/tcl8.6.14/library C:/tcl8.6.14/library
E       
E       C:/Miniconda/envs/idaes-pse-dev/Library/lib/tcl8.6/init.tcl: couldn't read file "C:/Miniconda/envs/idaes-pse-dev/Library/lib/tcl8.6/init.tcl": No error
E       couldn't read file "C:/Miniconda/envs/idaes-pse-dev/Library/lib/tcl8.6/init.tcl": No error
E           while executing
E       "source C:/Miniconda/envs/idaes-pse-dev/Library/lib/tcl8.6/init.tcl"
E           ("uplevel" body line 1)
E           invoked from within
E       "uplevel #0 [list source $tclfile]"
E       
E       
E       This probably means that Tcl wasn't installed properly.

C:\Miniconda\envs\idaes-pse-dev\lib\tkinter\__init__.py:2299: TclError

During handling of the above exception, another exception occurred:

alamo_surrogate = <idaes.core.surrogate.alamopy.AlamoSurrogate object at 0x0000020CEE61A7A0>
data_validation =     Bypass_Fraction  NG_Steam_Ratio  ...        N2            O2
0          0.171014        1.000000  ...  0.341778  2...4  ...  0.317834  3.530000e-20
19         0.262319        1.200000  ...  0.324708  3.670000e-20

[20 rows x 15 columns]

    @pytest.mark.unit
    def test_parity_alamo(alamo_surrogate, data_validation):
    
>       with TempfileManager.new_context() as tf:

idaes\core\surrogate\plotting\tests\test_alamo_plotting.py:109: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\pyomo\common\tempfiles.py:263: in __exit__
    self.release()
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\pyomo\common\tempfiles.py:420: in release
    self._remove_filesystem_object(name)
C:\Miniconda\envs\idaes-pse-dev\lib\site-packages\pyomo\common\tempfiles.py:469: in _remove_filesystem_object
    shutil.rmtree(name, ignore_errors=not deletion_errors_are_fatal)
C:\Miniconda\envs\idaes-pse-dev\lib\shutil.py:750: in rmtree
    return _rmtree_unsafe(path, onerror)
C:\Miniconda\envs\idaes-pse-dev\lib\shutil.py:620: in _rmtree_unsafe
    onerror(os.unlink, fullname, sys.exc_info())
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

path = 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp2gyo2l4n'
onerror = <function rmtree.<locals>.onerror at 0x0000020CF16B6B00>

    def _rmtree_unsafe(path, onerror):
        try:
            with os.scandir(path) as scandir_it:
                entries = list(scandir_it)
        except OSError:
            onerror(os.scandir, path, sys.exc_info())
            entries = []
        for entry in entries:
            fullname = entry.path
            if _rmtree_isdir(entry):
                try:
                    if entry.is_symlink():
                        # This can only happen if someone replaces
                        # a directory with a symlink after the call to
                        # os.scandir or entry.is_dir above.
                        raise OSError("Cannot call rmtree on a symbolic link")
                except OSError:
                    onerror(os.path.islink, fullname, sys.exc_info())
                    continue
                _rmtree_unsafe(fullname, onerror)
            else:
                try:
>                   os.unlink(fullname)
E                   PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp2gyo2l4n\\results.pdf'

C:\Miniconda\envs\idaes-pse-dev\lib\shutil.py:[618](https://github.com/IDAES/idaes-pse/actions/runs/11802661048/job/32878857667?pr=1519#step:7:619): PermissionError
============================== warnings summary ===============================

@andrewlee94
Copy link
Member

@dallan-keylogic That is a known ongoing but intermittent issue. The plotting tests really need to be revisited to see if there is anything we can actually test - at the moment they just test that the code executes with no failures, but occasionally it results in errors like this.

@lbianchi-lbl
Copy link
Contributor

@dallan-keylogic for reference, the issue @andrewlee94 is referring to is #1453.

@lbianchi-lbl
Copy link
Contributor

The Codecov upload failures are due to codecov/feedback#577, which should now be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-compat Affects backward compatibility deprecation Add deprecation to codebase and/or remove deprecated feature Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants