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

Update unit tests for allocate net gen #2297

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

jdangerx
Copy link
Member

@jdangerx jdangerx commented Feb 8, 2023

While talking about @grgmiller 's PR #2235, @cmgosnell and I decided to take a crack at testing the allocation behavior.

There's a few assertions that fail due to some bugs in our "allocating stuff that is missing in various places" code - I decided that fixing those bugs is out of scope of "get the tests working again."

TODO:

  • we didn't convert the other xfail test yet
  • we could probably come up with a cleaner looking way to provide input data
  • we could probably assert other things about the allocation beyond "the sums match"

@jdangerx jdangerx linked an issue Feb 8, 2023 that may be closed by this pull request
@jdangerx
Copy link
Member Author

@cmgosnell I got the urge to clean these up a bit more. Take a look! We should probably do some more complicated asserts, and maybe test the other top-level function (aggregate_gen_fuel_by_generator), but I think the pattern is now easier to follow.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Base: 86.0% // Head: 86.0% // Increases project coverage by +0.0% 🎉

Coverage data is based on head (bc1b936) compared to base (fcee14b).
Patch coverage: 100.0% of modified lines in pull request are covered.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #2297   +/-   ##
=====================================
  Coverage   86.0%   86.0%           
=====================================
  Files         74      74           
  Lines       9273    9273           
=====================================
+ Hits        7980    7981    +1     
+ Misses      1293    1292    -1     
Impacted Files Coverage Δ
src/pudl/analysis/allocate_net_gen.py 97.0% <100.0%> (+0.3%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@grgmiller
Copy link
Collaborator

I might suggest setting the threshold lower than 5% or 7%. In theory, the allocated totals should exactly match the input totals, although in practice perhaps rounding errors might create slight differences. Maybe a stricter 1% (or 0.5%?) threshold would catch situations where the allocation is off while allowing some small tolerance for rounding? Or should we test that they exactly match?

@jdangerx jdangerx marked this pull request as ready for review February 13, 2023 20:13
Copy link
Member

@cmgosnell cmgosnell left a comment

Choose a reason for hiding this comment

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

these changes definitely make this easier to read and navigate! Appreciate the fixture - that should make it easier to test different things. We have a long way to go to make this module less 🐛y but hopefully this will be a good place to build from.

Comment on lines +21 to +22
annual_2021 = 22_222.0
annual_2020 = 20_202.0
Copy link
Member

Choose a reason for hiding this comment

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

this is great! good use of the "don't make the full output dataframe" methodology

@@ -1875,10 +1876,11 @@ def test_original_gf_vs_the_allocated_by_gens_gf(
f"{data_col}: {off_by_5_perc:.1%} of allocated plant/year's are off by more"
" than 5%"
)
if off_by_5_perc > 0.07:
acceptance_threshold = 0.07
Copy link
Member

Choose a reason for hiding this comment

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

👶🏻 thing but this is now an arg so you don't need it in here

test/unit/analysis/allocate_net_gen_test.py Outdated Show resolved Hide resolved
It appears that they've found a bug in our allocation but I would
like to get these tests committed before fixing the bug.
Stop forcing docstrings in tests.
@jdangerx jdangerx force-pushed the daz-christina/unit-tests-for-allocate-net-gen branch from 7048f36 to bc1b936 Compare February 15, 2023 19:16
@jdangerx jdangerx merged commit c029539 into dev Feb 15, 2023
@jdangerx jdangerx deleted the daz-christina/unit-tests-for-allocate-net-gen branch February 15, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fix allocate_net_gen tests
3 participants