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.

Sign up for GitHub

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

Jump to bottom

Reactor_series #28

Merged
merged 15 commits into from
May 20, 2024
Merged

Reactor_series #28

merged 15 commits into from
May 20, 2024

Conversation

AlbertLee125
Copy link
Member

I added the reactor series with the incomplete documentation.

AlbertLee125
AlbertLee125 commented May 17, 2024
View reviewed changes
Copy link
Member Author

@AlbertLee125 AlbertLee125 left a comment

Choose a reason for hiding this comment

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

I ran the Python file using BARON with bigM reformulation. It gave the same results as my references in the dsda.

AlbertLee125
AlbertLee125 commented May 19, 2024
View reviewed changes
Copy link
Member Author

@AlbertLee125 AlbertLee125 left a comment

Choose a reason for hiding this comment

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

The documentation of the CSTR is completed. I think I need the review.

@AlbertLee125 AlbertLee125 added the documentation Improvements or additions to documentation label May 19, 2024
bernalde
bernalde requested changes May 19, 2024
View reviewed changes
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.

Missing file header, and several descriptions and types

gdplib/cstr/gdp_reactor.py Show resolved Hide resolved
gdplib/cstr/gdp_reactor.py Outdated Show resolved Hide resolved
gdplib/cstr/gdp_reactor.py Outdated Show resolved Hide resolved
gdplib/cstr/gdp_reactor.py Outdated Show resolved Hide resolved
gdplib/cstr/gdp_reactor.py Outdated Show resolved Hide resolved
gdplib/cstr/gdp_reactor.py
@@ -7,29 +7,21 @@


def build_cstrs(NT: int = 5) -> pyo.ConcreteModel():
Copy link
Member

Choose a reason for hiding this comment

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

This is the size parameter that would be good to vary for the benchmarking @ZedongPeng

@AlbertLee125 AlbertLee125 requested a review from bernalde May 19, 2024 21:34
bernalde
bernalde requested changes May 20, 2024
View reviewed changes
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.

Move the file header to the beginning and make sure that you have added all the descriptions and types in the files, not only where I commented.

gdplib/cstr/gdp_reactor.py Show resolved Hide resolved
gdplib/cstr/gdp_reactor.py
@@ -488,7 +500,7 @@ def YPD_rate_rel(disjunct):
Returns
-------
Pyomo.Constraint
_description_
Reaction rate relation for defining pyomo model.
Copy link
Member

Choose a reason for hiding this comment

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

There were many more instances where changes needed to be made. Please review this

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what I should do. Did you mean that I have to rewrite the returns?

@bernalde bernalde merged commit e2f5ede into master May 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bernalde bernalde bernalde approved these changes

@ZedongPeng ZedongPeng Awaiting requested review from ZedongPeng

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlbertLee125 @bernalde