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

Updated the HDA example exercise and solution #62

Merged
merged 33 commits into from
Aug 29, 2024

Conversation

JavalVyas2000
Copy link
Contributor

@JavalVyas2000 JavalVyas2000 commented Jul 10, 2023

Updated the hda_flowsheet_with_distillation_exercise.ipynb and hda_flowsheet_with_distillation_solution.ipynb files for the unit models initialization. Changed the function "function" used for initialization to accommodate for the newer initialization method "fix_initialization_states" where the inlet variables are fixed.


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

@JavalVyas2000
Copy link
Contributor Author

@lbianchi-lbl, I changed a couple of files in the HDA_flowsheet example. The files shows changes likes

  "attachments": {},
  "cell_type": "markdown",
  "metadata": {},
  "source": [ ... 

I am not sure why this is the case. The change that I actually did is around the line 993.

@lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl, I changed a couple of files in the HDA_flowsheet example. The files shows changes likes

  "attachments": {},
  "cell_type": "markdown",
  "metadata": {},
  "source": [ ... 

I am not sure why this is the case. The change that I actually did is around the line 993.

@JavalVyas2000 thanks for noticing and asking. Those metadata changes are automatically applied to .ipynb files by Jupyter when running a notebook.

This can be a little confusing and we've been discussing internally to set up some infrastructure so that they don't appear in the diff, but we don't have it set up yet, so for the time being, feel free to ignore them (both as an author and a reviewer).

@lbianchi-lbl lbianchi-lbl added the Priority:Normal Normal Priority Issue or PR label Jul 13, 2023
@lbianchi-lbl
Copy link
Contributor

I've just realized that this PR is making changes on the _exercise.ipynb notebook, which might or might not be compatible with how our framework is setup. @dangunter, can you chime in on this?

@lbianchi-lbl
Copy link
Contributor

@MAZamarripa @agarciadiego feel free to add some details about the scope and planned timeline for this work.

@bpaul4
Copy link
Contributor

bpaul4 commented Oct 17, 2023

@lbianchi-lbl I did a quick test of pre-processing a re-saved notebook, e.g. make a small change to hda_flowsheet_with_distillation.ipynb and see what idaex pre does. It created new versions of the _doc.ipynb, _exercise.ipynb, _solution.ipynb, _test.ipynb, and _usr.ipynb files. So I think the proper way to handle this is to update the main notebook and then preprocessing/build before checking in all of the new files.

@andrewlee94 I'll take a look at the Initilalizer docs and update the HDA with Distillation notebook. Once we're happy with that, I am happy to update other tutorial or example notebooks as needed and push them to this same PR.

@ksbeattie
Copy link
Member

@JavalVyas2000 @bpaul4 should this be on the Aug release board?

@JavalVyas2000
Copy link
Contributor Author

Yes. I am working on this. Should be done by then.

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.

Only a couple of minor requests for changes.

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, thanks @JavalVyas2000!

Copy link
Contributor

@agarciadiego agarciadiego left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewlee94 andrewlee94 merged commit 8f46f82 into IDAES:main Aug 29, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants