fix(farmer_registry_demo): unique owner-style farm names + household surname (#1114)#284
Merged
Merged
Conversation
…114)
The seeded generator drew ~730 farms from only 86 surnames as
"{surname} Farm", so farm names duplicated heavily — and because each
farm's registry ID value was derived from its name, the duplicates also
shared the same Tax ID / National ID.
Farms are now named after their owner ("{given} {family} {type word}",
e.g. Maria Santos Farm / Amir Mangudadatu Fishpond), which widens the
name space enough to be unique; residual collisions rotate the realistic
descriptor pool rather than appending a number. Registry ID values are
also salted with the partner id so they stay unique regardless of name.
The seeded RNG order remains deterministic.
…'s surname across members (#1114)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the OpenSPP Farmer Registry Demo generator to name each farm after its head member and assign the head's family name to all household members, while ensuring unique farm names and registry IDs. The reviewer identified a potential gender-name mismatch bug when head_gender is set to 'any', as the gender is resolved independently during name generation and member creation. To address this, the reviewer suggested propagating the resolved head gender from Phase 1 through Phase 3, updating all tuple unpacking locations and tests accordingly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+441
to
+443
| farm_name, head_given, family_name = self._generate_household_identity( | ||
| bp.get("farm_type"), bp.get("head_gender") | ||
| ) |
There was a problem hiding this comment.
When head_gender is "any", the head's name is chosen from a combined pool of male and female names. However, the head's gender is currently resolved independently in Phase 3, which can lead to gender-name mismatches (e.g., a male head named 'Maria'). We should propagate the resolved head gender from Phase 1 to Phase 3 to ensure consistency.
Suggested change
| farm_name, head_given, family_name = self._generate_household_identity( | |
| bp.get("farm_type"), bp.get("head_gender") | |
| ) | |
| farm_name, head_given, family_name, resolved_head_gender = self._generate_household_identity( | |
| bp.get("farm_type"), bp.get("head_gender") | |
| ) |
Comment on lines
+517
to
+519
| member_specs.append( | ||
| (bp, i, size, gps, group_phone, group_bank_name, group_acc_no, head_given, family_name) | ||
| ) |
There was a problem hiding this comment.
Propagate the resolved head gender to member_specs so it can be used during Phase 3.
Suggested change
| member_specs.append( | |
| (bp, i, size, gps, group_phone, group_bank_name, group_acc_no, head_given, family_name) | |
| ) | |
| member_specs.append( | |
| (bp, i, size, gps, group_phone, group_bank_name, group_acc_no, head_given, family_name, resolved_head_gender) | |
| ) |
Comment on lines
537
to
545
| for group_idx, (bp, _instance_idx, _size, _gps, _gphone, _gbank, _gacc, head_given, family_name) in enumerate( | ||
| member_specs | ||
| ): | ||
| group_record = groups[group_idx] | ||
| # Given names already used in this household — seeded with the head's | ||
| # so other members don't accidentally reuse it (OP#1114). | ||
| used_given = {head_given} | ||
| for member_spec in bp["members"]: | ||
| gender = self._resolve_gender(member_spec.get("gender", "any")) |
There was a problem hiding this comment.
Unpack the resolved head gender and use it to set the head member's gender, ensuring it matches the generated name.
Suggested change
| for group_idx, (bp, _instance_idx, _size, _gps, _gphone, _gbank, _gacc, head_given, family_name) in enumerate( | |
| member_specs | |
| ): | |
| group_record = groups[group_idx] | |
| # Given names already used in this household — seeded with the head's | |
| # so other members don't accidentally reuse it (OP#1114). | |
| used_given = {head_given} | |
| for member_spec in bp["members"]: | |
| gender = self._resolve_gender(member_spec.get("gender", "any")) | |
| for group_idx, (bp, _instance_idx, _size, _gps, _gphone, _gbank, _gacc, head_given, family_name, resolved_head_gender) in enumerate( | |
| member_specs | |
| ): | |
| group_record = groups[group_idx] | |
| # Given names already used in this household — seeded with the head's | |
| # so other members don't accidentally reuse it (OP#1114). | |
| used_given = {head_given} | |
| for member_spec in bp["members"]: | |
| if member_spec["role"] == "head": | |
| gender = resolved_head_gender | |
| else: | |
| gender = self._resolve_gender(member_spec.get("gender", "any")) |
| results = [] | ||
| ind_offset = 0 | ||
| for group_idx, (bp, _instance_idx, size, gps, _gphone, _gbank, _gacc) in enumerate(member_specs): | ||
| for group_idx, (bp, _instance_idx, size, gps, _gphone, _gbank, _gacc, _hg, _fam) in enumerate(member_specs): |
There was a problem hiding this comment.
Unpack the 10th element (resolved_head_gender) from member_specs to keep the unpacking consistent.
Suggested change
| for group_idx, (bp, _instance_idx, size, gps, _gphone, _gbank, _gacc, _hg, _fam) in enumerate(member_specs): | |
| for group_idx, (bp, _instance_idx, size, gps, _gphone, _gbank, _gacc, _hg, _fam, _gen) in enumerate(member_specs): |
| # ---- Phase: phone numbers ---- | ||
| phone_vals = [] | ||
| for group, (_bp, _i, _s, _g, gphone, _gb, _ga) in zip(groups, member_specs, strict=False): | ||
| for group, (_bp, _i, _s, _g, gphone, _gb, _ga, _hg, _fam) in zip(groups, member_specs, strict=False): |
There was a problem hiding this comment.
Unpack the 10th element (resolved_head_gender) from member_specs in the phone numbers phase.
Suggested change
| for group, (_bp, _i, _s, _g, gphone, _gb, _ga, _hg, _fam) in zip(groups, member_specs, strict=False): | |
| for group, (_bp, _i, _s, _g, gphone, _gb, _ga, _hg, _fam, _gen) in zip(groups, member_specs, strict=False): |
| bank_vals = [] | ||
| # One bank account per farm group. | ||
| for group, (_bp, _i, _s, _g, _gphone, gbank, gacc) in zip(groups, member_specs, strict=False): | ||
| for group, (_bp, _i, _s, _g, _gphone, gbank, gacc, _hg, _fam) in zip(groups, member_specs, strict=False): |
There was a problem hiding this comment.
Unpack the 10th element (resolved_head_gender) from member_specs in the bank accounts phase.
Suggested change
| for group, (_bp, _i, _s, _g, _gphone, gbank, gacc, _hg, _fam) in zip(groups, member_specs, strict=False): | |
| for group, (_bp, _i, _s, _g, _gphone, gbank, gacc, _hg, _fam, _gen) in zip(groups, member_specs, strict=False): |
| sp_by_name = {sp.name: sp.id for sp in sp_records} | ||
| if sp_by_name: | ||
| for group, (bp, _i, _s, _g, _gphone, _gb, _ga) in zip(groups, member_specs, strict=False): | ||
| for group, (bp, _i, _s, _g, _gphone, _gb, _ga, _hg, _fam) in zip(groups, member_specs, strict=False): |
There was a problem hiding this comment.
Unpack the 10th element (resolved_head_gender) from member_specs in the service point linkage phase.
Suggested change
| for group, (bp, _i, _s, _g, _gphone, _gb, _ga, _hg, _fam) in zip(groups, member_specs, strict=False): | |
| for group, (bp, _i, _s, _g, _gphone, _gb, _ga, _hg, _fam, _gen) in zip(groups, member_specs, strict=False): |
Comment on lines
+859
to
+905
| def _generate_household_identity(self, farm_type=None, head_gender=None): | ||
| """Generate a unique farm name tied to its head member (OP#1114). | ||
|
|
||
| Returns ``(farm_name, head_given, family_name)``. The farm is named after | ||
| its head — "{head_given} {family_name} {descriptor}", e.g. "Maria Santos | ||
| Farm" — and ``family_name`` is the surname shared by every member of the | ||
| household, so the group reads as a family of the head. The named owner is | ||
| therefore an actual member (the head), not an unrelated invented name. | ||
|
|
||
| Uniqueness of the farm name is preserved by rotating the realistic | ||
| descriptor pool and then re-rolling the owner — never a bare numeric | ||
| suffix. | ||
| """ | ||
| descriptors = _FARM_NAME_DESCRIPTORS.get(farm_type, _DEFAULT_FARM_DESCRIPTORS) | ||
| if head_gender == "female": | ||
| given_pool = _FILIPINO_FEMALE_FIRST_NAMES | ||
| elif head_gender == "male": | ||
| given_pool = _FILIPINO_MALE_FIRST_NAMES | ||
| else: | ||
| given_pool = _FILIPINO_MALE_FIRST_NAMES + _FILIPINO_FEMALE_FIRST_NAMES | ||
|
|
||
| given = self.rng.choice(given_pool) | ||
| family = self.rng.choice(_FILIPINO_LAST_NAMES) | ||
| for _ in range(30): | ||
| owner = f"{given} {family}" | ||
| # Skip owners reserved as story personas — the owner is now a real | ||
| # member, so it must not clash with a hand-authored persona name. | ||
| if owner not in self._reserved_names: | ||
| for descriptor in descriptors: | ||
| name = f"{owner} {descriptor}" | ||
| if name not in self._reserved_names and name not in self._used_names: | ||
| self._used_names.add(name) | ||
| return name, given, family | ||
| # Whole descriptor pool taken for this owner — re-roll the owner. | ||
| given = self.rng.choice(given_pool) | ||
| family = self.rng.choice(_FILIPINO_LAST_NAMES) | ||
| full_name = f"{given} {family}" | ||
| if full_name not in self._reserved_names: | ||
| return given, family | ||
| return given, family | ||
|
|
||
| # Exhausted realistic combinations (practically unreachable) — guarantee | ||
| # a unique value so demo generation never fails. | ||
| base = f"{given} {family} {descriptors[0]}" | ||
| name = base | ||
| suffix = 2 | ||
| while name in self._reserved_names or name in self._used_names: | ||
| name = f"{base} {suffix}" | ||
| suffix += 1 | ||
| self._used_names.add(name) | ||
| return name, given, family |
There was a problem hiding this comment.
Resolve the head's gender if it is "any" or None to prevent gender-name mismatches, and return it as the 4th element of the tuple. Also, keep track of the last valid non-reserved owner name during the 30 attempts to avoid returning a reserved owner name in the fallback suffix loop.
def _generate_household_identity(self, farm_type=None, head_gender=None):
"""Generate a unique farm name tied to its head member (OP#1114).
Returns ``(farm_name, head_given, family_name, resolved_gender)``. The farm is named after
its head — "{head_given} {family_name} {descriptor}", e.g. "Maria Santos
Farm" — and ``family_name`` is the surname shared by every member of the
household, so the group reads as a family of the head. The named owner is
therefore an actual member (the head), not an unrelated invented name.
Uniqueness of the farm name is preserved by rotating the realistic
descriptor pool and then re-rolling the owner — never a bare numeric
suffix.
"""
descriptors = _FARM_NAME_DESCRIPTORS.get(farm_type, _DEFAULT_FARM_DESCRIPTORS)
resolved_gender = head_gender
if resolved_gender not in ("male", "female"):
resolved_gender = self.rng.choice(["male", "female"])
if resolved_gender == "female":
given_pool = _FILIPINO_FEMALE_FIRST_NAMES
else:
given_pool = _FILIPINO_MALE_FIRST_NAMES
given = self.rng.choice(given_pool)
family = self.rng.choice(_FILIPINO_LAST_NAMES)
last_valid = None
for _ in range(30):
owner = f"{given} {family}"
# Skip owners reserved as story personas — the owner is now a real
# member, so it must not clash with a hand-authored persona name.
if owner not in self._reserved_names:
last_valid = (given, family)
for descriptor in descriptors:
name = f"{owner} {descriptor}"
if name not in self._reserved_names and name not in self._used_names:
self._used_names.add(name)
return name, given, family, resolved_gender
# Whole descriptor pool taken for this owner — re-roll the owner.
given = self.rng.choice(given_pool)
family = self.rng.choice(_FILIPINO_LAST_NAMES)
# Exhausted realistic combinations (practically unreachable) — guarantee
# a unique value so demo generation never fails.
if last_valid:
given, family = last_valid
base = f"{given} {family} {descriptors[0]}"
name = base
suffix = 2
while name in self._reserved_names or name in self._used_names:
name = f"{base} {suffix}"
suffix += 1
self._used_names.add(name)
return name, given, family, resolved_gender| given, family = gen._generate_member_name("male") | ||
| full = f"{given} {family}" | ||
| self.assertNotIn(full, gen._reserved_names) | ||
| name, head_given, family = gen._generate_household_identity("crop", "female") |
There was a problem hiding this comment.
Update the test to unpack the 4th element (resolved_gender) returned by _generate_household_identity to prevent a ValueError.
Suggested change
| name, head_given, family = gen._generate_household_identity("crop", "female") | |
| name, head_given, family, resolved_gender = gen._generate_household_identity("crop", "female") |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #284 +/- ##
==========================================
+ Coverage 68.23% 72.14% +3.91%
==========================================
Files 157 1010 +853
Lines 13420 60915 +47495
==========================================
+ Hits 9157 43948 +34791
- Misses 4263 16967 +12704
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…o head name matches gender_id (#1114)
gonzalesedwin1123
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change needed?
OP#1114 reported that the Farmer Registry demo data generated duplicated farm names and duplicated Tax/National IDs across households. Members within a single household also did not share a family name, so a farm did not read as one coherent family.
How was the change implemented?
The seeded farm generator now names each farm after its head member (e.g. "Maria Santos Farm") and gives every member the head's family name, so a household reads as one family. Farm names and registry IDs remain unique across the dataset, and generation stays seed-deterministic (same seed -> same output).
models/seeded_farm_generator.py: derive the farm name from the head member and propagate the head's family name to all members; keep farm names and registry IDs unique.New unit tests
tests/test_seeded_farm_generator.py: assert farm names are owner-style, members share the head's family name, and farm names / Tax / National IDs are unique.tests/test_blueprint_reproducibility.py: assert the generator remains seed-deterministic (reproducible output for a given seed).Unit tests executed by the author
spp_farmer_registry_demotest suite (seeded farm generator + blueprint reproducibility).How to test manually
spp_farmer_registry_demo.Related links
OP#1114