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

Translator block ASM1-ADM1 #962

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Conversation

agarciadiego
Copy link
Contributor

Summary/Motivation:

Translator block ASM1-ADM1

Changes proposed in this PR:

  • Includes a translator unit model to specific ASM1/ADM1 property packages

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.

@agarciadiego agarciadiego marked this pull request as draft March 14, 2023 21:43
@ksbeattie ksbeattie added the Priority:High High Priority Issue or PR label Mar 16, 2023
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.

Aside from lack of validation data, can you remind me what else is needed to take this out of draft mode? I think it's OK that we don't completely validate results in this PR and note in an issue to be addressed in a subsequent PR. We could also see if we can get data via this repo: https://github.com/wwtmodels/Benchmark-Simulation-Models

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #962 (07087ba) into main (a68abff) will increase coverage by 0.13%.
The diff coverage is 98.91%.

❗ Current head 07087ba differs from pull request most recent head aecd286. Consider uploading reports for the commit aecd286 to get more accurate results

@@            Coverage Diff             @@
##             main     #962      +/-   ##
==========================================
+ Coverage   95.47%   95.60%   +0.13%     
==========================================
  Files         285      289       +4     
  Lines       27244    27437     +193     
==========================================
+ Hits        26010    26230     +220     
+ Misses       1234     1207      -27     
Impacted Files Coverage Δ
...ap/unit_models/translators/translator_asm1_adm1.py 98.91% <98.91%> (ø)
...ap/unit_models/translators/translator_adm1_asm1.py 97.22% <100.00%> (ø)

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@agarciadiego agarciadiego marked this pull request as ready for review March 17, 2023 20:26
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- a couple comments, most of them trivial

Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tarnold17 tarnold17 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 to me

@lbianchi-lbl lbianchi-lbl merged commit d0ded58 into watertap-org:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants