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

Fixing typos and adding documentation to hda file #26

Merged
merged 13 commits into from
Jun 24, 2024
Merged

Fixing typos and adding documentation to hda file #26

merged 13 commits into from
Jun 24, 2024

Conversation

parkyr
Copy link
Contributor

@parkyr parkyr commented May 16, 2024

first round of documentation to hda file, including header and function documentation.
some typos were fixed as well in this pull request.

@bernalde
Copy link
Member

Because of the recent PR merged by @ZedongPeng there are conflicts in this branch now. I would suggest you to figure it out with @parkyr before getting this through review

@ZedongPeng
Copy link
Member

No problem. I will help Yirang resolve the conflicts.

@ZedongPeng
Copy link
Member

Great. You made it. @parkyr

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.

Some small changes are suggested but more importantly, since so many changes are made I want you to verify that the coeds still run. You have the original slide deck to compare.

gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
@parkyr
Copy link
Contributor Author

parkyr commented May 30, 2024

I ran the code after the changes. The code runs, but it's giving me a slightly different obj value from the slidedeck; objective value for slidedeck: 5966.51; the objective value result: 5801.27.

I compared it with the the original (unmodified) file from master branch, and the values match. The code was not affected from the documentation modifications.

@bernalde
Copy link
Member

This might arise from the subsolver that you are using, and whether it has improved since the time that previous table was made. Together with @ZedongPeng try to run all the different solvers to replicate the same results as in the table

gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
gdplib/hda/HDA_GDP_gdpopt.py Outdated Show resolved Hide resolved
@ZedongPeng
Copy link
Member

Hi @parkyr . Have you run the code before the changes? Does the optimal solution (before changes) equal 5966.51 or 5801.27?
You can go back to the old code with git checkout master.

@parkyr
Copy link
Contributor Author

parkyr commented May 31, 2024

Hi @ZedongPeng, yes I did run the code from the master branch and it gives me 5801.27. I'll do some digging and see if I can recreate while you are away.

@ZedongPeng
Copy link
Member

Hi @parkyr . Have you resolved this problem? I diff-checked the code line by line and didn't find any change that would result in a different result. Have you compared the log of GDPOpt before and after the changes? If you upload the logs in this PR, I can help you check them.

Btw, if you are using two different Python virtual environments in Anaconda (one for the hda branch and the other for the master branch), it might be worth checking if you are using the same version of Pyomo.

@parkyr
Copy link
Contributor Author

parkyr commented Jun 24, 2024

Seems like the metrics of the problem are different in README.md and the results.

README:

Problem vars Bool bin int cont cons nl disj disjtn
HDA Model 733 12 0 0 721 728 151 12 6
Obj: 5966.51

Results (master and hda branches):

Problem vars Bool bin int cont cons nl disj disjtn
HDA Model 721 12 0 0 709 728 151 12 6
Obj: 5801.27

Updates were made to README.md file:
- better obj value was found.
- the number of variables and continuous variables in the original file were incorrectly listed.
@parkyr
Copy link
Contributor Author

parkyr commented Jun 24, 2024

Seems like the metrics of the problem are different in README.md and the results.

README:
Problem vars Bool bin int cont cons nl disj disjtn
HDA Model 733 12 0 0 721 728 151 12 6
Obj: 5966.51

Results (master and hda branches):
Problem vars Bool bin int cont cons nl disj disjtn
HDA Model 721 12 0 0 709 728 151 12 6
Obj: 5801.27

Cross-checked with the presentation and confirmed the results table values are the correct ones.
Changes have been made in README.md to correct this.

@bernalde bernalde merged commit a619ed7 into master Jun 24, 2024
2 checks passed
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