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

Update documentation of kaibel model #27

Merged
merged 12 commits into from
Aug 19, 2024
Merged

Update documentation of kaibel model #27

merged 12 commits into from
Aug 19, 2024

Conversation

tristantc
Copy link
Contributor

@tristantc tristantc commented May 17, 2024

This pull request contains changes to the documentation of the ´kaibel_prop.py´ file.

@tristantc tristantc added the documentation Improvements or additions to documentation label May 17, 2024
@tristantc tristantc requested review from bernalde and ZedongPeng May 17, 2024 11:21
@tristantc tristantc changed the title Update documentation and units Update documentation and units of kaibel_prop.py May 17, 2024
@tristantc tristantc assigned tristantc and unassigned tristantc May 17, 2024
@tristantc tristantc marked this pull request as ready for review May 17, 2024 11:58
@bernalde
Copy link
Member

Please sort the conflict with the merged PRs from @ZedongPeng before we revise this

@tristantc tristantc changed the title Update documentation and units of kaibel_prop.py Update documentation of kaibel model May 19, 2024
Copy link
Member

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Good work. There are some lingering TODO and TOCHECK notes that should be addressed or rewritten.
The most important one is regarding an updated logical expression system in Pyomo. If the model does not run with the current version of Pyomo, we need to update it in this PR.
Finally, put all the units between square brackets

@@ -1,103 +1,204 @@
""" Side feed flash """
Copy link
Member

Choose a reason for hiding this comment

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

Can you add more documentation to this and the rest of the files at the header level? Check the other models for reference


@msf.Constraint(msf.nc,
doc="Vapor-liquid equilibrium constant")
# TODO: Is it computed using the Peng-Robinson equation of state?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this classifies as a TODO (as there is no action to be taken). I would add it as a Note. Moreover, I would encourage you to contact Soraya asking for these and other questions you might have

msf.OBJ = Objective(
expr=1,
sense=minimize)
# TODO: Is it computed using the Peng-Robinson equation of state?
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above

m.Tbot = m.TB0 # Bottom-most tray temperature in K
m.Ttop = m.TD0 # Top-most tray temperature in K
m.Tcon = m.TD0 - 5 # Condenser temperature in K
m.Treb = m.TB0 + 5 # Reboiler temperature in K
Copy link
Member

Choose a reason for hiding this comment

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

instead of "in UNITS" let's use "[UNITS]" to standardize with other models


m.flow_max = 1e3 # Flowrates upper bound in mol/s
Copy link
Member

Choose a reason for hiding this comment

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

See units comment above

for comp in m.comp:
m.dHvap[comp] = dHvapb[comp] / m.Hscale

## Heat capacity calculation for liquid and vapor phases using Ruczika-D method for each component in the feed, section, and tray
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a link or reference to the method to the header of the file?

>= m.min_num_trays
)

# TOCHECK: pyomo.GDP Syntax
Copy link
Member

Choose a reason for hiding this comment

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

For whom is this TOCHECK note?

else:
return Constraint.NoConstraint

# TOCHECK: Update the logic proposition constraint for the main section with the new pyomo.gdp syntax
Copy link
Member

Choose a reason for hiding this comment

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

Does the code run with the current version of Pyomo? If it doesn't then this update needs to be made in this PR

Copy link
Member

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Units should be added in many missing places and between [].
The most important comment is: does this run with the last version of Pyomo without warnings from the logical expression system?

gdplib/kaibel/kaibel_init.py Show resolved Hide resolved
gdplib/kaibel/kaibel_init.py Show resolved Hide resolved
gdplib/kaibel/kaibel_init.py Show resolved Hide resolved
mn.cols,
mn.sec,
mn.nc1,
doc='Temperature term for vapor pressure',
Copy link
Member

Choose a reason for hiding this comment

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

Missing units here and in many other places


from gdplib.kaibel.kaibel_side_flash import calc_side_feed_flash

# from .kaibel_side_flash import calc_side_feed_flash
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment

m.P = Var(
m.section,
m.tray,
doc="Pressure at each potential tray in bars",
Copy link
Member

Choose a reason for hiding this comment

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

Change to [units]

m.section,
m.tray,
m.comp,
doc="Liquid composition",
Copy link
Member

Choose a reason for hiding this comment

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

Missing units [mol/mol], similar below

@disj.Constraint(m.comp, doc="Top section 4 vapor enthalpy")
def top_vapor_enthalpy(disj, comp):
"""
Vapor enthalpy for the top section in the column.
Copy link
Member

Choose a reason for hiding this comment

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

Missing units here and in many other functions

m = build_model()

# Fixing variables
m.F[1].fix(50) # feed flowrate in mol/s
Copy link
Member

Choose a reason for hiding this comment

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

units in []

@bernalde
Copy link
Member

Does #47 supersede this? SHould we merge this beforehand?

@ZedongPeng ZedongPeng requested a review from bernalde August 19, 2024 16:32
@tristantc
Copy link
Contributor Author

Does #47 supersede this? SHould we merge this beforehand?

Requested changes resolved in branch #47, can you merge branch #27, @bernalde?

@bernalde bernalde merged commit afe6af4 into main Aug 19, 2024
2 of 3 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants