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

How-To Guide for Using a Property Model #500

Merged
merged 9 commits into from
Apr 27, 2022

Conversation

MarcusHolly
Copy link
Contributor

Fixes/Addresses:

Issue #275

Summary/Motivation:

This PR adds a How-To guide for using a property model.

Changes proposed in this PR:

  • Adds an .rst file to the list of How-To guides
  • Updates index file

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
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #500 (33cb6b9) into main (baba8d1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #500   +/-   ##
=======================================
  Coverage   94.72%   94.72%           
=======================================
  Files         171      171           
  Lines       14018    14018           
=======================================
  Hits        13279    13279           
  Misses        739      739           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baba8d1...33cb6b9. Read the comment docs.

@aladshaw3
Copy link
Collaborator

aladshaw3 commented Apr 26, 2022

Here is the log output from the doc test that failed.
FailedTest.txt

Looks like the test doesn't like the fact that you are displaying output...?

Copy link
Collaborator

@aladshaw3 aladshaw3 left a comment

Choose a reason for hiding this comment

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

A good start. Need to resolve the failing test (see suggestions above). Also may be nice to have a little more explanation in the beginning.

@aladshaw3 aladshaw3 added the documentation Improvements or additions to documentation label Apr 26, 2022
@aladshaw3 aladshaw3 self-requested a review April 26, 2022 16:58
@aladshaw3
Copy link
Collaborator

aladshaw3 commented Apr 26, 2022

Ok, so now only the test with python 3.8 is failing.
NewError_SameTest.txt

A warning is being output (see below) that is causing the current error.

"""
WARNING: The current pynumero_ASL library is version=3, but found version=2.
Please recompile / update your pynumero_ASL library.
"""

There should be a way in .rst files using testcode and testoutput where you can just ignore outputs. However, this may be an issue that needs to be resolved in the setup.py so that the GitHub actions are installing the most up to date libraries (I am not sure why this error is popping up only here).

Maybe @lbianchi-lbl can help out a bit more here.

@MarcusHolly
Copy link
Contributor Author

Ok, so now only the test with python 3.8 is failing. NewError_SameTest.txt

A warning is being output (see below) that is causing the current error.

""" WARNING: The current pynumero_ASL library is version=3, but found version=2. Please recompile / update your pynumero_ASL library. """

There should be a way in .rst files using testcode and testoutput where you can just ignore outputs.

Ok, thanks! I work part-time, so I'll look into this first thing tomorrow morning.

@adam-a-a
Copy link
Contributor

Ok, so now only the test with python 3.8 is failing. NewError_SameTest.txt

A warning is being output (see below) that is causing the current error.

""" WARNING: The current pynumero_ASL library is version=3, but found version=2. Please recompile / update your pynumero_ASL library. """

There should be a way in .rst files using testcode and testoutput where you can just ignore outputs. However, this may be an issue that needs to be resolved in the setup.py so that the GitHub actions are installing the most up to date libraries (I am not sure why this error is popping up only here).

Maybe @lbianchi-lbl can help out a bit more here.

I think @bknueven might have some ideas as well.

@bknueven
Copy link
Contributor

We're still using idaes-ext 2.5.5, but the runners are getting Pyomo 6.4, which contains a newer version of PyNumero.

I think the easiest thing is to resolve #503, going to at least 2.0.0a1 (or IDAES/idaes-pse@2cb6df0) and also enforce pyomo>6.3.0 in setup.py based on Pyomo/pyomo@e6e9b60.

adam-a-a
adam-a-a previously approved these changes Apr 27, 2022
Copy link
Contributor

@adam-a-a adam-a-a 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 to me! Nice job on your first PR, @MarcusHolly! I'm approving this even though we need to resolve the issue with the failed check.

@adam-a-a
Copy link
Contributor

@MarcusHolly - just one change I want to suggest (even though I approved already). Can you move how_to_use_a_property_model higher up in the index? I think it should be the second entry.

adam-a-a
adam-a-a previously approved these changes Apr 27, 2022
Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: bknueven <30801372+bknueven@users.noreply.github.com>
@aladshaw3 aladshaw3 requested review from bknueven and adam-a-a April 27, 2022 16:57
Copy link
Collaborator

@aladshaw3 aladshaw3 left a comment

Choose a reason for hiding this comment

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

Looks like all tests are passing now. I am not sure why there was that one python 3.8 failure before, but everything seems good now.

@bknueven bknueven merged commit b9a7d79 into watertap-org:main Apr 27, 2022
@aladshaw3 aladshaw3 mentioned this pull request Apr 28, 2022
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants