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

RDRP-1142_freezing_amendments #418

Merged
merged 20 commits into from
Feb 7, 2025
Merged

Conversation

Ryan2Y79
Copy link
Collaborator

Pull Request submission

Insert detailed bullet points about your changes here!

Insert any instructions to help the reviewer, e.g. "install new requirements from requirements.txt"

*Let the reviewer know what data files are needed (and if applicable, where they are to be found)

Closes or fixes

  • Detail the ticket(s) you are closing with this PR
    Closes #

Code

  • Code runs The code runs on my machine and/or CDSW
  • Conflicts resolved There are no conflicts (I have performed a rebase if necessary)
  • Requirements My/our code functions according to the requirements of the ticket
  • Dependencies I have updated the environment yaml so it includes any new libraries I have used
  • Configuration file updated any high level parameters that the user may interact with have been put into the config file (and imported to the script)
  • Clean Code
    • Code is as PEP 8 compliant as I can humanly make it
    • Code passess flake8 linting check
    • Code adheres to DRY
  • Type hints All new functions have type hints

Documentation

Any new code includes all the following forms of documentation:

  • Function Documentation Docstrings within the function(s')/methods have been created
    • Includes Args and returns for all major functions
    • The docstring details data types
  • Updated Documentation: User and/or developer working doc has been updated

Data

  • All data needed to run this script is available in Dev/Test
  • All data is excluded from this pull request
  • Secrets checker pre-commit passes

Testing

  • Unit tests Unit tests have been created and are passing or a new ticket to create tests has been created

Peer Review Section

  • All requirements install from (updated) requirements.txt
  • Documentation has been created and is clear - check the working document
  • Doctrings (Google format) have been created and accurately describe the function's functionality
  • Unit tests pass, or if not present a new ticket to create tests has been created
  • Code runs The code runs on reviewer's machine and/or CDSW

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction.

  • I recommend merging this request.

Review comments

Insert detailed comments here!

These might include, but not exclusively:

  • bugs that need fixing (does it work as expected? and does it work with other code
    that it is likely to interact with?)
  • alternative methods (could it be written more efficiently or with more clarity?)
  • documentation improvements (does the documentation reflect how the code actually works?)
  • additional tests that should be implemented (do the tests effectively assure that it
    works correctly?)
  • code style improvements (could the code be written more clearly?)
  • Do the changes represent a change in functionality so the version number should increase? Start a discussion if so.
  • As a review you can generates the same outputs from running the code

Your suggestions should be tailored to the code that you are reviewing.
Be critical and clear, but not mean. Ask questions and set actions.

Copy link
Collaborator

@AnneONS AnneONS left a comment

Choose a reason for hiding this comment

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

could we use this as an opportunity to replace this list with getting the cols from the config?

Also, the tests seem to be failing....

"706",
"707",
"709",
"711",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks rough, doesn't it? Can you find a way of using what we have in the dev config to get this list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deriving this list from the dev_config.yaml would be a great challenge. The reason is that this list of columns comes from an excel file shared between Sarah M and Jen C. I would suggest, as you have mentioned, moving this list to the config.

Copy link
Collaborator

@AnneONS AnneONS left a comment

Choose a reason for hiding this comment

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

consistency_checks:
2xx_totals:
purchases_split: ["222", "223", "203"]
sal_oth_expend: ["202", "203", "204"]
research_expend: ["205", "206", "207", "204"]
capex: ["219", "220", "209", "210"]
intram: ["204", "210", "211"]
funding: ['212', '214', '216', '242', '250', '243', '244', '245', '246', '247', '248', '249', '218']
ownership: ['225', '226', '227', '228', '229', '237', '218']
equality: ['211', '218']
3xx_totals:
purchases: ['302', '303', '304', '305']
4xx_totals:
emp_civil: ['405', '407', '409', '411']
emp_defence: ['406', '408', '410', '412']
5xx_totals:
hc_res_m: ['501', '503', '505', '507']
hc_res_f: ['502', '504', '506', '508']

Use this part of the config

and this code in utils/ breakdown_validation.py

def get_equality_dicts(config: dict, sublist: str = "default") -> dict:
"""
Get the equality checks for the construction data.

Args:
    config (dict): The config dictionary.

Returns:
    dict
"""
# use the config to get breakdown totals
all_checks_dict = config["consistency_checks"]

# isolate the relationships suitlable for checking in the construction module
if sublist == "default":
    wanted_dicts = [key for key in all_checks_dict.keys() if "xx_totals" in key]

@@ -30,6 +30,9 @@ freezing_paths:
freezing_changes_to_review_path: "changes_to_review"
freezing_amendments_path: "freezing_updates"
freezing_additions_path: "freezing_updates"
freezing_columns_to_check:
non_numeric_columns: ["200", "201", "601", "604"]
non_numeric_columns: ["202", "203", "204", "205", "206", "207", "209", "210", "211", "212", "214", "216", "218", "219", "220", "221", "222", "223", "225", "226", "227", "228", "229", "237", "242", "243", "244", "245", "246", "247", "248", "249", "250", "405", "406", "407", "408", "409", "410", "411", "412", "501", "502", "503", "504", "505", "506", "507", "508", "602", "701", "702", "703", "704", "705", "706", "707", "709", "711"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Ryan,

Instead of this, please could you use the code I put in my second review? Then we've got everything for the questions in the same place, and if we need to update, we only need to update in one place, thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course, no worries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some columns will ne hard coding in the config as they do not appear within config["consistency_checks"], but are specified by the business area to checked, for example 219.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the use of config["consistency_checks"] for gathering the columns freezing amendments is applied to:

Employing get_equality_dicts to gather columns for freezing comparison yields a comprehensive list of all values within config["consistency_checks"]. Splitting this list to match numeric and non-numeric columns to be checked (defined by the business area) requires either in the config or a script to contain these numeric and non-numeric column names, thus making the entire idea of removing hardcoded numeric and non-numeric from the config/script redundant.

Copy link

github-actions bot commented Jan 30, 2025

Percentage Coverage for this PR

Detailed Coverage Report
FileStmtsMissCoverMissing
src
   __init__.py00100% 
src/construction
   __init__.py00100% 
   all_data_construction.py47470%1, 3–4, 6, 8, 16, 22, 45–46, 49, 54–55, 59–61, 65–66, 69, 76–78, 83, 86–90, 92, 97–98, 103–104, 107–109, 113, 117–120, 125–126, 128, 133–134, 138, 140
   construction_helpers.py1041486%32–43, 240, 307
   construction_main.py27270%3–4, 6, 8, 12–13, 16, 19, 55–58, 65, 69–72, 75, 79–82, 85, 90–92, 94
   construction_read_validate.py34340%3–4, 6, 9, 12, 17, 19, 22, 47–50, 52–53, 56–57, 60, 67–69, 73–74, 80, 83, 102–104, 106–107, 110, 117–118, 123, 130
   construction_validation.py102991%84, 87, 114, 122–123, 128, 139, 169, 219
   postcode_construction.py21210%1–2, 4–6, 9, 26, 29, 32, 37, 40–41, 44, 47–49, 51, 55, 59, 62–63
src/estimation
   __init__.py00100% 
   apply_weights.py18288%47–48
   calculate_weights.py580100% 
   estimation_main.py22220%3–5, 7–9, 11, 14, 30, 36, 39, 42–43, 45–52, 54
src/freezing
   __init__.py00100% 
   freezing_apply_changes.py702170%31–34, 37–38, 41–43, 46–49, 53, 61–64, 68–69, 71
   freezing_compare.py722269%114–115, 163–164, 188, 191, 194–196, 200–202, 204–206, 208–209, 241, 264, 267, 270, 273
   freezing_main.py46460%1–3, 5, 7–12, 15, 18, 42–43, 46–47, 49–53, 55, 63, 66–68, 71–72, 76–78, 83–84, 86–87, 90–91, 95, 98, 109–112, 114–116
   freezing_utils.py110100% 
src/imputation
   MoR.py150596%66, 290, 442, 488–489
   __init__.py00100% 
   apportionment.py361072%125, 127, 142, 144, 146, 158–160, 163, 165
   expansion_imputation.py373018%22–23, 25–26, 29, 32–34, 38, 41, 43, 46–47, 49, 52, 55, 75–77, 81, 84, 87, 90, 96, 101, 104, 106, 112, 115, 119
   imputation_helpers.py1605764%64, 66, 82, 152–154, 163, 165–166, 168–169, 341, 343–344, 348–349, 351, 355, 376, 379, 383, 386–387, 389, 408, 410, 417–418, 420, 505, 507, 510–511, 514, 526–527, 530–531, 533, 535, 551–552, 555–556, 558, 624–625, 628, 631–634, 637, 640, 643–644, 646
   imputation_main.py71710%3–6, 8–18, 21, 24, 55, 58, 61–62, 65, 67, 72–73, 76, 79, 81–82, 84–85, 88, 92–94, 97, 101–102, 105–106, 109–111, 113, 117, 120–122, 124–128, 131–133, 135–137, 139–141, 144, 147, 150–155, 157
   impute_civ_def.py984752%165, 168, 198, 200–201, 204–205, 208–210, 215–216, 219, 222, 224–227, 230, 233, 238–239, 242, 245, 247–249, 252, 255, 258, 261, 263, 265, 267, 280, 283–287, 289–290, 293–294, 296, 298, 300
   manual_imputation.py19190%1–2, 4, 6, 9–10, 28–29, 33, 35, 37, 44, 59, 61–62, 64–65, 67–68
   sf_expansion.py715818%25, 28, 31–32, 35–36, 39, 42, 45–46, 49, 51, 58–59, 62, 67–68, 71, 73, 76–77, 79, 81, 84, 88, 90, 101, 104, 108, 110–111, 113, 115, 118, 121, 130, 133, 136, 145, 148, 150, 156, 162, 174, 177, 182–183, 185, 193–194, 197, 201, 205, 208, 216, 218, 222, 224
   short_to_long.py210100% 
   tmi_imputation.py16211330%26, 31, 36, 158, 161, 163, 166, 170, 174, 196–197, 200, 202, 208, 211–212, 215, 217–218, 220, 222, 225–226, 228, 230, 233, 236–237, 239, 242–244, 246–249, 251, 267–268, 270–271, 273–274, 276, 278, 280, 282, 285, 288–289, 292, 294, 296, 314–315, 317, 319–321, 323–324, 327, 329–330, 333, 335–336, 354, 356, 358, 362, 364–365, 368, 370–371, 373–374, 394–396, 399, 402–403, 407, 409, 428–429, 432, 434, 437, 440, 444, 448, 451, 453, 455, 457–458, 460, 462, 466, 473–474, 479, 482–483, 485, 488, 491, 496, 498–499
src/mapping
   __init__.py00100% 
   cellno_mapping.py17476%50–52, 54
   itl_mapping.py100100% 
   mapping_helpers.py59198%85
   mapping_main.py50500%3–5, 7–15, 17, 20, 45, 50, 53, 63, 72, 79, 82, 89, 92–94, 101, 103, 106–108, 111, 113–115, 118, 121, 126–127, 130, 132–136, 138–140, 143, 146, 149
   pg_conversion.py40880%52, 55, 58, 122, 125, 128, 162–163
   pnp_mapping.py660%1, 4, 7, 15, 31, 33
   ultfoc_mapping.py34488%49–51, 95
src/northern_ireland
   __init__.py00100% 
   ni_headcount_fte.py29290%3–5, 7, 9, 12, 26, 28, 30, 32, 34, 36, 38, 41, 56, 58–60, 62, 64–65, 67–68, 70, 73, 82, 84–85, 87
   ni_main.py21210%3–9, 11, 14, 34, 37–39, 45, 52–54, 62, 64–65, 67
   ni_staging.py33330%3–6, 8–9, 11, 14, 22, 25, 27–29, 31, 34, 40, 44, 46, 49, 78–79, 82, 87, 91, 94, 97–102, 104, 106
src/outlier_detection
   __init__.py00100% 
   auto_outliers.py900100% 
   manual_outliers.py160100% 
   outlier_main.py39390%3–5, 7–9, 11, 14, 43, 45–47, 49–50, 53, 56, 59–64, 66, 71–72, 75, 80–83, 86–90, 92, 95, 97, 99
src/outputs
   __init__.py00100% 
   export_files.py92920%4–10, 12–13, 17, 23, 41, 43, 50–53, 60, 67, 72, 75, 102, 108, 111, 118, 120, 123, 129, 131–132, 135–139, 142–143, 146, 157–159, 161, 164, 177, 179–180, 182, 185, 205, 208, 211–212, 219, 223, 226–228, 231, 233, 235, 237–238, 240, 243–247, 249–250, 252, 255–257, 260, 263, 266, 281, 284–285, 293, 296, 298, 300, 310, 312–314, 323, 325, 328–329
   form_output_prep.py181327%25–27, 29–30, 34, 36–37, 39, 41, 43, 47, 49
   frozen_group.py43430%3–4, 6, 8–11, 13, 16, 45, 47–50, 53, 71, 123, 134, 161, 164–168, 170, 172, 175, 181–182, 184, 187, 190, 195–196, 199–200, 203, 206–208, 211–212, 214
   gb_sas.py23230%3–5, 7–10, 12, 15, 31, 34, 39, 42, 45, 48, 53, 56, 59–61, 64–65, 67
   intram_by_civil_defence.py210100% 
   intram_by_itl.py541572%44–47, 51, 141, 144, 147–149, 152–153, 157–158, 167
   intram_by_pg.py350100% 
   intram_by_sic.py33330%3–6, 8, 11, 31–32, 35–36, 39–40, 42, 45–47, 52, 68–72, 74, 77, 80, 83, 86, 89–90, 93–94, 97, 99
   intram_totals.py14140%3–4, 6–7, 9, 12, 29–30, 33, 36, 39–40, 42–43
   long_form.py17170%3–5, 7–10, 12, 15, 30, 33, 36, 39–41, 43–44
   manifest_output.py82820%1–5, 9, 12–13, 16, 34, 50–53, 56–57, 61–62, 67–68, 76, 79–83, 86–92, 94, 112–113, 118, 120–121, 126, 129, 131, 133, 135, 139, 149, 154–155, 161, 164–165, 167–168, 174–177, 183–185, 187, 194, 196, 201, 203–205, 207–208, 210–211, 213–216, 218, 221, 223, 229–230, 233–234
   map_output_cols.py38781%151–152, 154, 156, 159, 162, 164
   ni_sas.py19190%3–9, 11, 14, 27, 30, 33, 36, 41, 44–46, 49–50
   outputs_helpers.py170100% 
   outputs_main.py83830%4–5, 8, 11–23, 25, 28, 50, 55–57, 62, 65, 68–70, 75, 78–79, 82–84, 90, 93–95, 101, 104–106, 108–109, 114, 117–119, 128, 131–133, 137–138, 147, 150–152, 159, 162–164, 168–169, 177, 180–182, 189, 192–194, 199, 202–204, 211, 214–216, 218–220, 222
   short_form.py341361%79, 86, 88, 105, 108, 111, 114, 117, 120–122, 124–125
   tau.py25250%3–9, 11, 14, 31, 35, 38, 41, 44, 47, 52, 55, 57–58, 61–63, 66–67, 69
   total_fte.py12120%3–6, 9, 12, 27, 29–30, 36, 41–42
src/site_apportionment
   __init__.py00100% 
   output_status_filtered.py261542%26, 28, 32–33, 35–36, 38, 45–46, 73, 76, 78, 80–81, 83
   site_apportionment.py126397%467, 469–470
   site_apportionment_main.py25250%3–5, 7–8, 12, 14, 17, 39–40, 43, 45, 48–49, 57–58, 60–61, 64, 69–72, 74–75
src/staging
   __init__.py00100% 
   postcode_validation.py56394%131–132, 220
   spp_parser.py140100% 
   spp_snapshot_processing.py340100% 
   staging_helpers.py881582%181, 184, 186, 189–190, 193–194, 196–197, 200, 204–205, 207, 211, 217
   staging_main.py1001000%4–6, 8, 10–12, 16, 19, 65–67, 70, 72, 76–78, 80–81, 85, 87–88, 91, 94, 97–98, 100–101, 106–108, 112–115, 117–118, 124, 129–131, 133–134, 137, 149–151, 156, 159, 163, 165, 167–169, 172, 176, 178, 180–184, 187, 190, 192–193, 196, 198, 201–205, 208, 212–213, 215, 217–219, 221–223, 225, 228, 236, 245–246, 248–253, 255, 257–258, 260, 263, 274
   validation.py1434965%34–36, 39, 42, 89–90, 101, 123, 136–137, 142, 146, 154–155, 160–161, 202–203, 209, 211, 214, 216, 223–224, 303, 305–306, 309–310, 313–314, 317, 320–321, 324, 326, 328–329, 343, 345–351, 354, 357
src/utils
   __init__.py00100% 
   breakdown_validation.py1334665%39, 196–198, 201, 203–207, 209, 211, 214–215, 221–222, 225, 228, 230, 233, 235, 247–250, 253–256, 272–273, 278, 321–322, 324, 330–331, 373–376, 378–381, 384
   config.py171795%152–153, 432, 493–496
   defence.py200100% 
   helpers.py731875%20–21, 25–26, 28, 135–138, 140–141, 144–146, 155, 157–158, 216
   local_file_mods.py1204760%47–53, 98, 148–150, 199–203, 214–215, 226, 237, 248–249, 251, 262–263, 274–275, 284, 292, 303, 305–306, 308–309, 313–315, 319, 333–334, 336–337, 340–341, 343, 360, 365
   path_helpers.py142894%97, 312, 314, 316–317, 319–320, 322
   postcode_reduction_helper.py38380%12, 14–15, 17, 20, 22, 25, 27–28, 30–31, 34–35, 38, 40–41, 43–44, 47, 60, 62, 65, 68, 71–72, 74–75, 77, 80, 95, 97, 100–101, 104–105, 107, 109–110
   s3_mods.py1271270%20–21, 23–24, 26, 34, 39–41, 45, 59, 61–62, 65–68, 70–71, 74, 85, 88, 93, 96–97, 100, 111–112, 114, 117, 131, 133–134, 136, 139, 149, 156, 159, 161, 164, 166, 169, 180–181, 183, 186, 194–195, 198, 207–212, 215, 226–227, 230–231, 234, 237, 240, 251–252, 254–257, 259–260, 263, 269, 272, 285, 288, 291, 293, 296, 302, 305, 308–309, 312, 320–322, 324, 327, 331–332, 334, 337, 366, 370, 372, 379, 382, 387–389, 396, 399, 415, 417–418, 420–421, 425, 427, 429, 432, 434–436, 438, 441, 451, 454–455, 458, 461, 463–464, 466–468
   singleton_boto.py20200%5–6, 9–11, 13–14, 16–23, 25–29
TOTAL4017188253% 

Summary of tests

Tests Skipped Failures Errors Time
287 2 💤 0 ❌ 0 🔥 3.502s ⏱️

Copy link
Collaborator

@AnneONS AnneONS left a comment

Choose a reason for hiding this comment

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

a few thoughts... I haven't finished reviewing yet.

}
}

return config
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you think it would be useful to have more columns to check? One col is not much....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it not needed. The unit test already checks 4 columns. Before my changes the unit test only checked 3 (and 3 must have been deemed satisfactory).

Copy link
Collaborator

@AnneONS AnneONS left a comment

Choose a reason for hiding this comment

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

a few more comments.

Copy link
Collaborator

@AnneONS AnneONS left a comment

Choose a reason for hiding this comment

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

more comments on test

"consistency_checks": {
"2xx_totals": {
"purchases_split": [],
"sal_oth_expend": ["203"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

add 202 ?

@AnneONS AnneONS merged commit 93b8183 into develop Feb 7, 2025
4 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.

2 participants