Skip to content

Conversation

@alee5646
Copy link
Contributor

Summary

Files updated:

Reason for change: What was the problem?
Gender to sex update in column name

Changes made:

- updated name of column from gender to sex 
- added trim function for sex column 

Checks for creator

Delete all non-relevant lines below, and then tick off as completed:

  • [ x] I have run the code in the scripts I have changed
  • [ x] I have run the relevant run_data/run_tables file
  • [ x] I have included comments to explain my code
  • [ x] I have checked the layout of the code is logical and easy to follow
  • [ x] I have checked new variables are named consistently

Closing issues

Replace the XXX with the issue numbers for any issues that can now be closed:

Close #XXX

Checks for reviewer

Creator - delete any not relevant. Reviewer - tick as completed:

  • I have run the code in the scripts that have changed
  • I have run the relevant run_data/run_tables file
  • I have checked the layout of the code is logical and easy to follow
  • I have checked new variables are named consistently
  • I have checked the list of github issues

@s-french
Copy link
Contributor

@s-french to review

Copy link
Contributor

@s-french s-french left a comment

Choose a reason for hiding this comment

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

I'm getting an error in line 88, saying unit 'M' is not supported. Having a google, 'm' is minutes. Not sure how this is worked before, but chatgpt is suggesting the following:

✅ Solution: Use pd.DateOffset or divide by approximate month length
🔹 Option 1: Use .dt.to_period() or .dt.months logic (recommended if you're comparing calendar months)

If you're trying to get the number of full months between two dates:

dol_table['ordspan_months'] = (
(dol_table['ordfindate'].dt.to_period('M') - dol_table['orderdate'].dt.to_period('M'))
).apply(lambda x: x.n)

Explanation:

.dt.to_period('M') converts dates to monthly periods (e.g., 2024-01).

Subtracting periods gives a MonthEndOffset, which .n converts to an integer.

This gives calendar-month difference, rounded down (e.g. Jan 31 to Feb 1 = 1 month).

@alee5646
Copy link
Contributor Author

@s-french Line 88 error corrected and added code to redact any genders that is not male/female and group under "other"

@alee5646
Copy link
Contributor Author

Checked new CSV without region column against old output, same results in QA file, left a copy in the HC DOL update folder - "DoL Table 2025 Q3 - region removed"

Copy link
Contributor

@s-french s-french left a comment

Choose a reason for hiding this comment

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

Hi @alee5646, have looked and it runs fine so good work fixing that M issue. There are a few things below that need addressing as really the code should be clean re region/gender rather than fixes added towards the end. We've been the victim of code where patches are applied and it easily builds up to get very unclear code so would appreciate this file being cleaned up as outlined below

Code chunk 7 (importing region lookup) is redundant so should be deleted

Code chunk 8 doesn't need to join to the region lookup so needs amending

Ditto chunk 9

Chunks 10-12,14-17 refers to gender rather than sex - can now see this is dealt with in chunk 20. Can't say the shortcut is preferred, better to fix from the start (a ctrl+f should find all references easily)

Chunk 12 still includes 'child region of DoL' - suspect this isn't needed (subsequent queries may need adapting accordingly)

@s-french
Copy link
Contributor

Also, just noted that the Q3 version of the csv still has region in it so needs fixing

@alee5646
Copy link
Contributor Author

Also, just noted that the Q3 version of the csv still has region in it so needs fixing

I was hanging on swapping the final CSV file, just in the new one needed changes, have swapped the final CSV over now

@alee5646
Copy link
Contributor Author

Removed party_type from output, commented out region lookup join for now, will come back and tidy up areas

Copy link
Contributor

@s-french s-french left a comment

Choose a reason for hiding this comment

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

all looks good for the DoLs workbook thanks

There are a number of divorce checkpoint files in this PR as well - I don't know the impact of merging these in tbh so can you remove them please? I'll then merge once clear (possibly a result of using 'git add .' which adds all files changed in that session rather than naming the specific files changed?)

@alee5646 alee5646 force-pushed the HC_dol_trim_gender_update branch from f266c6a to 3155ee4 Compare January 14, 2026 09:26
@alee5646 alee5646 force-pushed the HC_dol_trim_gender_update branch from 3155ee4 to ab80904 Compare January 14, 2026 09:42
@alee5646
Copy link
Contributor Author

Yea I think I accidently did git add data and pushed the whole folder instead of just the DoL folder, little bit of a pain to get rid of the checkpoint files but figured out a method in the end - stash dol changes and merge development into the branch (make sure local is up to date!) and bring the stashed dol changes back in, everything looks ok now with only changes in the DoL folder

@s-french s-french merged commit 18374c8 into development Jan 14, 2026
@s-french s-french deleted the HC_dol_trim_gender_update branch January 14, 2026 14:34
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