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

Keras file format updates #128

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Conversation

bpaul4
Copy link
Contributor

@bpaul4 bpaul4 commented Jul 17, 2024

Fixes #88.

Dependent on IDAES/idaes-pse#1401 for tests to pass.


Legal Acknowledgement

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

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

📚 Documentation preview 📚: https://idaes-examples--128.org.readthedocs.build/en/128/

@bpaul4 bpaul4 self-assigned this Jul 17, 2024
@ksbeattie ksbeattie added the Priority:High High Priority Issue or PR label Jul 18, 2024
@@ -443,16 +444,30 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"Now we have our property package ready for being used in the flowsheet for optimization. We shall see that in the next part of this tutorial, [flowsheet_optimization](./flowsheet_optimization_usr.ipynb). To learn in detail about making a custom property package, one should go through [Property Package Example](../../../properties/custom/custom_physical_property_packages_usr.ipynb). "
Copy link
Contributor

@lbianchi-lbl lbianchi-lbl Jul 18, 2024

Choose a reason for hiding this comment

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

@bpaul4 was this change required because of a specific error? Usually I'd expect (but haven't actually checked for Jupyter/JupyterLab) that Unix-style paths with / are accepted on Windows as well, so I'd be curious to know if there might be something that warranted this change.

Copy link
Contributor Author

@bpaul4 bpaul4 Jul 18, 2024

Choose a reason for hiding this comment

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

That's very odd. The double slash change doesn't exist on my local branch; there's another change in "_doc" version of this notebook from ".ipynb" to ".md" that I am sure I did not make. Not sure how this happened, as the other versions should all be identical to the source notebook, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, that's very strange. I'll try to look into it, starting from attempting to reproduce this locally.

@lbianchi-lbl lbianchi-lbl self-requested a review July 18, 2024 20:46
Comment on lines 3 to 5
# idaes-pse @ git+https://github.com/IDAES/idaes-pse@main
# FIXME: revert back to IDAES/idaes-pse@main before merging
idaes-pse @ git+https://github.com/IDAES/idaes-pse@refs/pull/1401/merge
Copy link
Contributor

Choose a reason for hiding this comment

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

FIXME: revert this before merging

Copy link
Contributor

@lbianchi-lbl lbianchi-lbl left a comment

Choose a reason for hiding this comment

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

Just a reminder to revert the changes to requirements-dev.txt before merging.

@lbianchi-lbl
Copy link
Contributor

We should proceed as follows:

  1. Wait until Keras file format updates idaes-pse#1401 is merged
  2. Revert the change to the requirements file causing IDAES to be installed from the PR branch
  3. Merge this in (assuming tests pass)

@bpaul4
Copy link
Contributor Author

bpaul4 commented Aug 16, 2024

The 3.8 tests appear stuck, and everything else is passing. @lbianchi-lbl is the plan to remove 3.8 support in this repository, and remove the 3.8 tests?

FYI per @andrewlee94 we probably need to merge this PR first, and then 1401 in idaes-pse.

@lbianchi-lbl
Copy link
Contributor

lbianchi-lbl commented Aug 16, 2024

The 3.8 tests appear stuck, and everything else is passing. @lbianchi-lbl is the plan to remove 3.8 support in this repository, and remove the 3.8 tests?

FYI per @andrewlee94 we probably need to merge this PR first, and then 1401 in idaes-pse.

Good point @bpaul4, yes, let me open a PR for that.

EDIT Actually, on second thought, I'll just update and merge this PR since I think the changes in this PR are needed to make tests pass with Python 3.12.

@lbianchi-lbl lbianchi-lbl mentioned this pull request Aug 16, 2024
3 tasks
@lbianchi-lbl lbianchi-lbl merged commit e0d3966 into IDAES:main Aug 16, 2024
1 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve Keras File Format Issues
3 participants