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

OARO 1D Model #1172

Merged
merged 92 commits into from
Nov 9, 2023
Merged

OARO 1D Model #1172

merged 92 commits into from
Nov 9, 2023

Conversation

luohezhiming
Copy link
Contributor

Fixes/Resolves:

This PR is to add OARO 1D model for issue #1131
This PR inherits @bknueven previous revisions of initialization routine #848 and make all tests passed.

Summary/Motivation:

Create OARO 1D model

Changes proposed in this PR:

  • add OARO 1D model
  • add tests

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

adam-a-a and others added 30 commits November 17, 2022 16:43
Comment on lines 1195 to 1196
# badly_scaled_var_lst = list(badly_scaled_var_generator(m))
# assert badly_scaled_var_lst == []
Copy link
Contributor

Choose a reason for hiding this comment

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

uncomment or delete

Copy link
Contributor

@adam-a-a adam-a-a 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! Can you update documentation as well?

Copy link
Contributor

@zacharybinger zacharybinger left a comment

Choose a reason for hiding this comment

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

LGTM

@adam-a-a adam-a-a enabled auto-merge (squash) November 8, 2023 21:12
@adam-a-a
Copy link
Contributor

adam-a-a commented Nov 9, 2023

@luohezhiming just realized my last comment still would need to be addressed, but we can do that in a subsequent PR. I'll create an issue to track adding/updating OARO documentation

This was referenced Nov 9, 2023
@adam-a-a
Copy link
Contributor

adam-a-a commented Nov 9, 2023

@luohezhiming - since we merged the SKK PR in first, that is breaking tests here. Hopefully, it's as easy as increasing the number of config options expected and nothing more than that.

@luohezhiming
Copy link
Contributor Author

@luohezhiming just realized my last comment still would need to be addressed, but we can do that in a subsequent PR. I'll create an issue to track adding/updating OARO documentation

Hi, Adam. I've already updated the issue for documentation in issue #1131

@luohezhiming
Copy link
Contributor Author

@luohezhiming - since we merged the SKK PR in first, that is breaking tests here. Hopefully, it's as easy as increasing the number of config options expected and nothing more than that.

Yeah, it seems just config number issue.

@adam-a-a
Copy link
Contributor

adam-a-a commented Nov 9, 2023

@luohezhiming just realized my last comment still would need to be addressed, but we can do that in a subsequent PR. I'll create an issue to track adding/updating OARO documentation

Hi, Adam. I've already updated the issue for documentation in issue #1131

Thanks for the reminder! I forgot about that. Deleting the redundant issue that I created then.

@adam-a-a adam-a-a merged commit 4c2f3fa into watertap-org:main Nov 9, 2023
24 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.

5 participants