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

Remove CBO elasticities toggle #5347

Merged
merged 9 commits into from
Dec 5, 2024

Conversation

vrathi101
Copy link
Collaborator

Fixes #5310

@vrathi101 vrathi101 marked this pull request as draft November 19, 2024 17:23
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 99.03%. Comparing base (cb92a56) to head (568ec5c).
Report is 215 commits behind head on master.

Files with missing lines Patch % Lines
...ion/labor_supply_response/labor_supply_response.py 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5347      +/-   ##
==========================================
- Coverage   99.12%   99.03%   -0.10%     
==========================================
  Files        2592     2628      +36     
  Lines       37707    38223     +516     
  Branches      162      165       +3     
==========================================
+ Hits        37378    37854     +476     
- Misses        297      335      +38     
- Partials       32       34       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PavelMakarchuk PavelMakarchuk marked this pull request as ready for review November 19, 2024 22:45
Copy link
Contributor

@MaxGhenis MaxGhenis left a comment

Choose a reason for hiding this comment

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

Could you also add unit tests while here to appease the codecov?

@MaxGhenis
Copy link
Contributor

Friendly ping

@vrathi101
Copy link
Collaborator Author

vrathi101 commented Dec 4, 2024

Friendly ping

Me and @PavelMakarchuk were working through issues with how to write the unit tests for labor_supply_response.py because of some input issues with the class structure

@PavelMakarchuk
Copy link
Collaborator

Friendly ping

Me and @PavelMakarchuk were working through issues with how to write the unit tests for labor_supply_response.py because of some input issues with the class structure

@MaxGhenis Yes I would file a separate issue to add testing for labor supply related variables as we can't seem to accommodate the requires_computation_after metadata

Copy link
Contributor

@MaxGhenis MaxGhenis left a comment

Choose a reason for hiding this comment

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

please fix the unit of the elasticities per my previous comment

@vrathi101 vrathi101 requested a review from MaxGhenis December 4, 2024 23:12
@MaxGhenis
Copy link
Contributor

Responses should stay USD, elasticities are /1

@MaxGhenis MaxGhenis merged commit 6deafbe into PolicyEngine:master Dec 5, 2024
3 of 5 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.

Remove CBO elasticities toggle
3 participants