Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

Adding Methanol Synthesis Flowsheet Example #88

Closed
wants to merge 1 commit into from

Conversation

bpaul4
Copy link
Contributor

@bpaul4 bpaul4 commented Jan 19, 2022

Fixes # .

Proposed changes:

  • Adding methanol synthesis property and reaction packages, flowsheet and PFD to examples repository

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.

@bpaul4 bpaul4 self-assigned this Jan 19, 2022
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Jan 20, 2022
@ksbeattie ksbeattie added this to In progress in 2022 February idaes-pse release via automation Jan 20, 2022
@andrewlee94
Copy link
Member

Seeing as this is just some python scripts and not an actual notebook, I have to ask if this really belongs here, or if it should be somewhere in idaes-pse (in an examples folder somewhere).

If it is something we want here, then I think we should have a proper notebook, and link it to the doc tree properly (at the moment it will not show up). I will also note that the png file is currently just there, and doesn't do anything,

@bpaul4
Copy link
Contributor Author

bpaul4 commented Feb 7, 2022

@MAZamarripa If it would be useful to spend time on this, I can convert this flowsheet (and/or the version with a recycle) into a fully-fledged Jupyter notebook and properly update the doctree. Otherwise, we could add this to idaes-pse/idaes/generic_models/flowsheets or create a new folder in idaes-pse as @andrewlee94 suggests.

@bpaul4
Copy link
Contributor Author

bpaul4 commented Feb 11, 2022

@andrewlee94 @MAZamarripa Thank you for your suggestions regarding example notebook best practices. I have completed a draft of this notebook and will use this PR to add it to examples-pse. The imported modules will be added to idaes\generic_models\flowsheets and the imported properties and reactions packages will be added to idaes\generic_models\properties\examples via a PR to idaes-pse.

In addition to updating the doc-tree in examples-pse, I would like to know your thoughts on updating the idaes-pse doc-tree to add a new section Technical Specifications > Generic IDAES Model Libaries > Flowsheets and creating RST files for each flowsheet to place in docs\apidocs.

@andrewlee94
Copy link
Member

@bpaul4 For anything being added to idaes-pse it will need documentation. You should probably work with @FiannaOBrien to work out what is needed and where. Note that the api-docs are generally autogenerated without any input.

@FiannaOBrien
Copy link
Collaborator

@bpaul4 I agree with adding documentation of the Flowsheets to idaes-pse. In the new re-org, this page would now be under "Reference Guides" > "Model Libraries" > "Generic IDAES Model Libaries" > "Flowsheets". As for writing this up, here's the documentation on writing a reference guide from the Diataxis Framework.
As for the code in the PR, I really appreciate the use of in-line comments to explain what's going on. I might also suggest adding comment blocks within the beginning of each function to explain what the goal of the function is.

@bpaul4
Copy link
Contributor Author

bpaul4 commented Feb 14, 2022

Thank you @FiannaOBrien, I appreciate your feedback and will adhere to the standards defined in your new reorganization. I'll update the doctree where needed and open a new PR for the IDAES-PSE files.

@bpaul4
Copy link
Contributor Author

bpaul4 commented Feb 15, 2022

I am closing this PR since these commits are from an outdated branch. Please see #98 for updated commits and discussion.

@bpaul4 bpaul4 closed this Feb 15, 2022
2022 February idaes-pse release automation moved this from In progress to Done Feb 15, 2022
@bpaul4 bpaul4 deleted the methanol_synthesis branch August 18, 2023 16:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Priority:Normal Normal Priority Issue or PR
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants