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

Allow initialization of CarsonsEquations from a dictionary. #31

Closed
wants to merge 2 commits into from

Conversation

etimberg
Copy link
Contributor

@etimberg etimberg commented Oct 2, 2019

This change eliminates the need to always construct a model object to use Carson's equations. In the future, we can simplify the test code to use these dictionaries instead of classes.

Resolves #30

@etimberg etimberg added the enhancement New feature or request label Oct 2, 2019
@etimberg etimberg requested a review from AnjoMan October 2, 2019 00:18
Copy link
Member

@AnjoMan AnjoMan left a comment

Choose a reason for hiding this comment

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

I like it!

@etimberg
Copy link
Contributor Author

@AnjoMan I fixed the linter issue

@coveralls
Copy link

Pull Request Test Coverage Report for Build 167

  • 21 of 26 (80.77%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.5%) to 85.976%

Changes Missing Coverage Covered Lines Changed/Added Lines %
carsons/carsons.py 21 26 80.77%
Totals Coverage Status
Change from base Build 150: -1.5%
Covered Lines: 141
Relevant Lines: 164

💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 15, 2019

Pull Request Test Coverage Report for Build 206

  • 24 of 29 (82.76%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.4%) to 86.391%

Changes Missing Coverage Covered Lines Changed/Added Lines %
carsons/carsons.py 24 29 82.76%
Totals Coverage Status
Change from base Build 197: -1.4%
Covered Lines: 292
Relevant Lines: 338

💛 - Coveralls

This change eliminates the need to always construct a model object
to use Carson's equations. In the future, we can simplify the test code
to use these dictionaries instead of classes.
@etimberg
Copy link
Contributor Author

@AnjoMan do you want me to rebase this? it's ancient but I see there's some activity on the project again

@AnjoMan
Copy link
Member

AnjoMan commented Oct 14, 2022

@etimberg i'm not sure it matters too much -- to merge, we'd have to add additional logic to cover the tape-shield stuff that just got merged in, and i'm guessing it wouldn't end up getting used anyways

@etimberg etimberg closed this Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Carsons models to avoid double class creation
3 participants