Skip to content

Conversation

@dayantur
Copy link

@dayantur dayantur commented Nov 30, 2025

While addressing #851 , I noticed ArchetypeProperties params were not nullified in phase_b.py when stebbsmethod ==0.
When stebbsmethod ==0, both stebbs and building_archetype params must be set to null.

This PR also edits all ArchetypeProperties Pydantic fields to become Optional (allowing for null values).

@dayantur dayantur self-assigned this Nov 30, 2025
@dayantur dayantur temporarily deployed to github-pages-preview November 30, 2025 22:10 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Nov 30, 2025

🔍 Schema Preview Deployed

Preview URLs:

Production URLs (unchanged):


⚠️ Important: Preview schemas are in a subdirectory and do not affect production. The preview pages include warning banners to prevent accidental use in production configs.

@dayantur dayantur requested a review from sunt05 November 30, 2025 22:21
@dayantur dayantur temporarily deployed to github-pages-preview December 2, 2025 12:39 — with GitHub Actions Inactive
@sunt05
Copy link

sunt05 commented Dec 2, 2025

PR Review Summary

Overall Assessment: ✅ Approve with minor suggestions

Category Status Notes
Code Logic ✅ PASS Correctly fixes the nullification gap
Code Style ✅ PASS Follows SUEWS conventions
Type Safety ✅ PASS Optional types properly applied
CI/Build ✅ PASS All checks passing
Documentation ⚠️ MINOR CHANGELOG entry missing PR reference
Test Coverage ⚠️ MINOR No test for building_archetype nullification

Key Findings

✅ What's Good

  1. Core fix is correct: When stebbsmethod == 0, both stebbs and building_archetype blocks should be nullified. The fix properly addresses this by iterating over both block names.

  2. Improved robustness: The refactored _nullify_block helper now handles lists in addition to dicts, which is more defensive against unexpected data structures.

  3. Clean refactoring: The nested function _nullify_block improves code readability and reduces duplication compared to the original inline logic.

  4. Optional types are appropriate: Making ArchetypeProperties fields Optional allows them to accept None values, which is necessary for the nullification logic to work correctly.

⚠️ Minor Suggestions

  1. CHANGELOG: Add PR number (#958) for traceability. (see line comment)

  2. Test coverage: The existing test test_stebbsmethod0_nullifies_all_stebbs_values() only tests stebbs block. Consider adding a companion test for building_archetype:

def test_stebbsmethod0_nullifies_building_archetype_values():
    yaml_input = {
        "model": {"physics": {"stebbsmethod": {"value": 0}}},
        "sites": [{"properties": {
            "building_archetype": {
                "BuildingType": {"value": "SampleType"},
                "stebbs_Height": {"value": 10.0},
            }
        }}],
    }
    result = precheck_model_option_rules(deepcopy(yaml_input))
    out = result["sites"][0]["properties"]["building_archetype"]
    assert out["BuildingType"]["value"] is None
    assert out["stebbs_Height"]["value"] is None

Suggested Reviewers


1 line comment posted for specific issues

🤖 Generated with Claude Code

Copy link

@sunt05 sunt05 left a comment

Choose a reason for hiding this comment

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

See inline comment on CHANGELOG.md

@dayantur dayantur temporarily deployed to github-pages-preview December 3, 2025 11:44 — with GitHub Actions Inactive
@dayantur dayantur temporarily deployed to github-pages-preview December 3, 2025 11:44 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Dec 3, 2025

Documentation Preview

Preview URL: https://umep-dev.github.io/SUEWS/preview/pr-958/docs/

This is a PR preview. Production docs remain at https://suews.readthedocs.io

…also building_archetype nullification

and fixed precheck_model_option_rules to include building_archetype in the nullification logic
@dayantur dayantur temporarily deployed to github-pages-preview December 3, 2025 12:01 — with GitHub Actions Inactive
Co-authored-by: dayantur <71443948+dayantur@users.noreply.github.com>
@github-actions
Copy link

github-actions bot commented Dec 3, 2025

🤖 I've automatically formatted the code in this PR using:

  • Python: ruff v0.8.6
  • Fortran: fprettify v0.3.7

Please pull the latest changes before making further edits.

@dayantur
Copy link
Author

dayantur commented Dec 3, 2025

Hi @sunt05 - test added & changelog PR specification included :)
hi @yiqing1021 - can you give a thumb up as well if everything looks fine?

@dayantur dayantur enabled auto-merge December 3, 2025 12:23
@dayantur dayantur disabled auto-merge December 3, 2025 14:12
@dayantur dayantur enabled auto-merge December 3, 2025 14:12
@dayantur dayantur disabled auto-merge December 3, 2025 15:51
@dayantur dayantur temporarily deployed to github-pages-preview December 3, 2025 15:54 — with GitHub Actions Inactive
@dayantur dayantur temporarily deployed to github-pages-preview December 3, 2025 15:54 — with GitHub Actions Inactive
@dayantur dayantur enabled auto-merge December 3, 2025 15:57
@dayantur dayantur added this pull request to the merge queue Dec 3, 2025
Merged via the queue into master with commit 0436faf Dec 3, 2025
17 checks passed
@dayantur dayantur temporarily deployed to github-pages-preview December 3, 2025 16:51 — with GitHub Actions Inactive
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.

3 participants