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

refactor: core functionality into class and methods #75

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kdheepak
Copy link
Contributor

@kdheepak kdheepak commented Oct 17, 2024

This PR abstracts the core functionality into the following:

    sim = Simulation(mTEPES, DirName, CaseName, SolverName, pIndOutputResults, pIndLogConsole)

    sim.input_data()

    sim.define_variables()

    sim.objective_function_and_investment_constraints()

    sim.initialize_pduals_and_na()

    if sim.is_pcm():
        sim.build_and_solve_pcm()
    else:
        sim.build_and_solve_capex()

    sim.output_results()

@kdheepak kdheepak force-pushed the kd/refactor-core branch 2 times, most recently from ffb1b12 to b5b3915 Compare October 17, 2024 22:22
@kdheepak
Copy link
Contributor Author

kdheepak commented Oct 17, 2024

@erikfilias @arght I made two commits in this PR. The first commit retains the exact order of all operations from the master branch, and just refactors the code into individual methods. The second commit moves the check for sim.is_pcm() outside of the for loop, which allows introducing two more methods and rewriting it as:

    if sim.is_pcm():
        sim.build_and_solve_pcm()
    else:
        sim.build_and_solve_capex()

I chose method names based on the comments + what I thought made sense, but that is completely open to change. I noticed that the code in the master branch uses camelCase for the naming convention, but I picked snake_case instead of camelCase just to differentiate between the new methods introduced in this PR and existing functionality. That can be changed too.

# Define variables
SettingUpVariables(self.mTEPES, self.mTEPES)

def setup_first_last_stage(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

The set mTEPES.strepresents different stages, typically structured as "st" + number. However, this set is unordered, which means that the initial and final stages may vary with each execution, even when using identical input data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We changed everything to an ordered set in #66. So the sorting of pyomo should handle this correctly here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we removed both ordered=True and ordered=False. Perhaps, it might be more effective to determine the first and last stages based on the position of each stage's initial time step within the overall collection of time steps considered in the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, the default in pyomo is ordered=True, so when we removed the keyword argument we made everything ordered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification @kdheepak. If the default in Pyomo is indeed ordered=True, then it should handle the stage ordering as expected. Let me know if you think we should run additional tests to verify this.

Comment on lines +63 to +64
self.First_st = self.mTEPES.st.first()
self.Last_st = self.mTEPES.st.last()
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to my previous comment, the first st should include the initial time step of the year when aggregating all time steps across stages. Similarly, the final st should represent the last time step. @arght, please let me know if there are any corrections needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I introduced mTEPES.First_st and mTEPES.Last_st was to allow periods where the first local levels do not correspond to the first hours of the year, which may happen if we assign duration 0 to the starting hours. For this reason I wrote these instructions
# first/last stage
FirstST = 0
for st in mTEPES.st:
if FirstST == 0:
FirstST = 1
mTEPES.First_st = st
mTEPES.Last_st = st
instead of using .first() and .last() constructions from Pyomo. mTEPES.st only have the active stages (duration <>0) from the stage set.
I don't know if this clarifies the question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants