refactor: extract filter_by_anzsic_prefixes() shared helper by martinguthrie93 · Pull Request #200 · openmethane/openmethane-prior

martinguthrie93 · 2026-07-02T02:52:30Z

Summary

Following maintainer feedback on the GIS preprocessing discussion, this PR addresses one duplication (Rule of Three): identical ANZSIC prefix filtering in three modules.

Extracted filter_by_anzsic_prefixes() in data_sources/safeguard/anzsic.py.

Behaviour

No change to emission calculations or filtering results.

Test plan

uv run python -m pytest tests/unit/test_data/test_anzsic.py -v - 2 passed

Checklist

Tests added
Documentation added (not required)
Changelog - will add after MR number assigned

Made with Cursor

Replace identical ANZSIC prefix filtering in NPI, Safeguard, and oil and gas site sources with a single function in safeguard/anzsic.py. No change to emission calculations or filtering behaviour. Co-authored-by: Cursor <cursoragent@cursor.com>

EOF Co-authored-by: Cursor <cursoragent@cursor.com>

aethr

Change seems reasonable, but if we're going to make an abstraction/helper it has a higher burden of testing and documentation than something in a function body that isn't meant to be re-used.

Happy to accept this PR if we add a bit more diverse testing and documentation around the new function.

aethr · 2026-07-02T04:58:43Z

+    df = pd.DataFrame({"anzsic_code": ["0600", "1212", "0700", "9999"]})
+    filtered = filter_by_anzsic_prefixes(df, ["06", "07"], column="anzsic_code")
+    assert list(filtered["anzsic_code"]) == ["0600", "0700"]


I would suggest broadening this test to cover more complex matching. ANZSIC code "0600" and "06" are equivalent so that's a clear match. "0700" and "07" are similar, so adding them doesn't really broaden the coverage of the test.

For example, filtering by "06" should match:

06

060

0600

061

0612

Filtering by "202" might be more nuanced:

match "202"

match "2021"

match "2029"

doesn't match "20"

doesn't match "201"

doesn't match "22"

etc

Do any of the new tests actually test the prefix shortening? Might be good to explicitly have something for anzsic_codes: ["2000"] matching "2021", etc?

aethr · 2026-07-02T05:13:45Z

+    *,
+    column: str,
+) -> pd.DataFrame:
+    """Return rows whose ``column`` value starts with any simplified ANZSIC prefix."""


The ANZSIC classification scheme doesn't use the word "prefix", so if the aim is to make a helper that's more approachable we should try to use the right terminology. Since the anzsic_codes argument supports subdivision, group or class-level codes, I would probably call this something like filter_by_anzsic_code_family.

Providing at least one example for anzsic_codes and what resulting codes would match might be a good addition.

aethr · 2026-07-02T05:25:31Z

The failure in the Build docker image step is expected, since this is an external PR we don't want it to push images to our hosted packages.

Use ANZSIC code family terminology, expand unit tests, and document matching examples in the helper docstring. Co-authored-by: Cursor <cursoragent@cursor.com>

martinguthrie93 · 2026-07-02T06:07:27Z

Thanks for the review @aethr — addressed in ffc08a8:

Renamed helper to filter_by_anzsic_code_family (ANZSIC code family terminology)
Expanded unit tests for coal mining (06) and nuanced 202 matching
Added docstring examples for matching behaviour

All ANZSIC unit tests pass locally. Ready for re-review.

martinguthrie93 · 2026-07-02T06:35:59Z

Hi @aethr — thank you for the thoughtful review.

I have pushed ffc08a8 with your requested changes:

Renamed to filter_by_anzsic_code_family
Broader unit tests (06 and 202 matching cases)
Docstring examples for code family matching

All ANZSIC tests pass locally.

I also read your note that the Docker build failure is expected for external PRs. That makes sense. If possible, could the workflow be adjusted so fork PRs skip the ghcr.io push step and avoid showing a red failing check? It was a little confusing before I saw your explanation.

Ready for re-review — thanks again!

aethr · 2026-07-02T07:16:32Z

If possible, could the workflow be adjusted so fork PRs skip the ghcr.io push step and avoid showing a red failing check? It was a little confusing before I saw your explanation.

Raised in issue #201. This is possibly the first external PR to this repo, so not something we've run into before!

aethr

One small nitpick but otherwise looks good.

aethr · 2026-07-02T07:25:47Z

+    Each entry in ``anzsic_codes`` may be a division, subdivision, group, or class
+    code. Codes are simplified (trailing zeros removed) before matching, so filtering
+    by ``"06"`` matches ``"06"``, ``"060"``, ``"0600"``, ``"061"``, and ``"0612"``.
+    Filtering by ``"202"`` matches ``"202"``, ``"2021"``, and ``"2029"`` but not
+    ``"20"``, ``"201"``, or ``"22"``.


Nitpick, but technically "division" is not supported here (and it's rarely used in datasets I've seen). Division is like "C Manufacturing", a subdivision of which might be "11 Food Product Manufacturing". These helpers are only concerned with the numeric codes from subdivision onwards.

More detail available here: https://www.abs.gov.au/statistics/classifications/australian-and-new-zealand-standard-industrial-classification-anzsic/2006-revision-2-0/numbering-system-and-titles

Suggested change

Each entry in ``anzsic_codes`` may be a division, subdivision, group, or class

code. Codes are simplified (trailing zeros removed) before matching, so filtering

by ``"06"`` matches ``"06"``, ``"060"``, ``"0600"``, ``"061"``, and ``"0612"``.

Filtering by ``"202"`` matches ``"202"``, ``"2021"``, and ``"2029"`` but not

``"20"``, ``"201"``, or ``"22"``.

Each entry in ``anzsic_codes`` may be a subdivision, group, or class code

Codes are simplified, so specifying a subdivision like "0600" will match

any group or class within it. For example:

- ``"0600"`` or ``"06"`` matches ``"06"``, ``"060"``, ``"0600"``, ``"061"``, and ``"0612"``

- ``"202"`` matches ``"202"``, ``"2021"``, and ``"2029"`` but not

``"20"``, ``"201"``, or ``"22"``.

martinguthrie93 · 2026-07-02T07:26:11Z

Thanks again @aethr, and glad the fork-PR check issue was useful — happy to have surfaced it. The rename to filter_by_anzsic_code_family and the family-matching examples make the helper much clearer; appreciate you pushing on the terminology.

If #201 turns into something a contributor can pick up (e.g. gating the ghcr.io push step on non-fork PRs), I'd be glad to take a look and open a follow-up PR. Either way, thanks for the thorough and welcoming review on what turned out to be the first external PR here.

aethr

Reading my own comment re: the tests made me realise the pre-existing code has a bug in it.

In the classification, "Subdivision: 30 - Building Construction" is completely separate from "Subdivision: 31 - Heavy and Civil Engineering Construction", but if someone specified anzsic_codes: ["30"], the simplify_anzsic_code method would shorten it to "3", which would match all the subdivisions in "E Construction".

The simplify_anzsic_code method shouldn't remove any zeroes that occur in the first two characters. Would you mind updating the method and expanding the tests for both simplify_anzsic_code and filter_by_anzsic_code_family so this is covered?

Preserve the first two subdivision characters when removing trailing zeros, expand ANZSIC helper tests, and apply the requested docstring. Co-authored-by: Cursor <cursoragent@cursor.com>

martinguthrie93 · 2026-07-02T07:41:23Z

Thanks @aethr — addressed in the latest push:

Fixed simplify_anzsic_code() so zeros in the first two subdivision characters are preserved (e.g. 30 no longer becomes 3)
Added subdivision boundary, padded-code, and 2000/2021 family tests
Applied the docstring nitpick

All test_anzsic, test_safeguard, and test_npi unit tests pass locally. Ready for re-review.

aethr · 2026-07-02T07:41:32Z

If #201 turns into something a contributor can pick up (e.g. gating the ghcr.io push step on non-fork PRs), I'd be glad to take a look and open a follow-up PR.

I appreciate the offer, but infrastructure changes are probably best left to team members for now.

martinguthrie93 · 2026-07-02T07:53:07Z

Makes sense — that's a reasonable place to draw the line, and I'd rather infra stay with the team. Thanks for clarifying.

Distinguish subdivision-level (2000/20) from group-level (2020/202) matching behaviour in filter_by_anzsic_code_family tests. Co-authored-by: Cursor <cursoragent@cursor.com>

martinguthrie93 · 2026-07-02T08:00:51Z

Clarified padded-code tests to distinguish hierarchy levels:

subdivision: 2000 / 20 match 20, 201, 202, 2021, 2029
group: 2020 / 202 match 202, 2021, 2029 only (exclude 20, 201, 22)

aethr · 2026-07-09T01:18:52Z

Hi @martinguthrie93 , sorry I've been quiet on this PR. I've been working on resolving the issue with checks failing for external PRs in #202 and I was hoping we could test those improvements in this PR before merging. I'll comment again here when that's ready.

martinguthrie93 · 2026-07-09T01:37:25Z

Thanks for the update @aethr — no problem at all. Happy to wait while #202 is tested on this PR. Let me know if you need anything from my side.

HoHo and others added 2 commits July 2, 2026 03:25

Add changelog fragment for PR openmethane#200.

d59b97d

EOF Co-authored-by: Cursor <cursoragent@cursor.com>

aethr requested changes Jul 2, 2026

View reviewed changes

Address review feedback: rename to filter_by_anzsic_code_family.

ffc08a8

Use ANZSIC code family terminology, expand unit tests, and document matching examples in the helper docstring. Co-authored-by: Cursor <cursoragent@cursor.com>

martinguthrie93 requested a review from aethr July 2, 2026 06:32

aethr mentioned this pull request Jul 2, 2026

Review GHA workflows that run for external PRs #201

Open

2 tasks

aethr approved these changes Jul 2, 2026

View reviewed changes

aethr requested changes Jul 2, 2026

View reviewed changes

Fix simplify_anzsic_code subdivision zero stripping bug.

81c1cee

Preserve the first two subdivision characters when removing trailing zeros, expand ANZSIC helper tests, and apply the requested docstring. Co-authored-by: Cursor <cursoragent@cursor.com>

Clarify padded ANZSIC code family tests by hierarchy level.

a27c6c6

Distinguish subdivision-level (2000/20) from group-level (2020/202) matching behaviour in filter_by_anzsic_code_family tests. Co-authored-by: Cursor <cursoragent@cursor.com>

Uh oh!

Conversation

martinguthrie93 commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behaviour

Test plan

Checklist

Uh oh!

aethr left a comment

Choose a reason for hiding this comment

Uh oh!

aethr Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

aethr Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

aethr Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

aethr commented Jul 2, 2026

Uh oh!

martinguthrie93 commented Jul 2, 2026

Uh oh!

martinguthrie93 commented Jul 2, 2026

Uh oh!

aethr commented Jul 2, 2026

Uh oh!

aethr left a comment

Choose a reason for hiding this comment

Uh oh!

aethr Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

martinguthrie93 commented Jul 2, 2026

Uh oh!

aethr left a comment

Choose a reason for hiding this comment

Uh oh!

martinguthrie93 commented Jul 2, 2026

Uh oh!

aethr commented Jul 2, 2026

Uh oh!

martinguthrie93 commented Jul 2, 2026

Uh oh!

martinguthrie93 commented Jul 2, 2026

Uh oh!

aethr commented Jul 9, 2026

Uh oh!

martinguthrie93 commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

martinguthrie93 commented Jul 2, 2026 •

edited

Loading