Skip to content

Fix OR vs AND logic error in get_model#593

Open
aymuos15 wants to merge 1 commit intobrainglobe:mainfrom
aymuos15:fix-get-model-logic
Open

Fix OR vs AND logic error in get_model#593
aymuos15 wants to merge 1 commit intobrainglobe:mainfrom
aymuos15:fix-get-model-logic

Conversation

@aymuos15
Copy link
Copy Markdown

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

The original condition in get_modelif existing_model is not None or network_depth is None — has a logic error. When both existing_model and network_depth are None, the or evaluates to True (because network_depth is None), causing load_model(None) to be called, which crashes with an unhelpful Keras error.

What does this PR do?

  • Replaces the compound or condition with a clear if / elif / else chain so that existing_model always takes precedence when provided
  • Adds an explicit ValueError guard when neither existing_model nor network_depth is supplied
  • Updates the docstring to clearly describe parameter precedence rules
  • Adds tests for the existing-model-takes-precedence path, the depth-only build path, and the both-None error case

References

None.

How has this PR been tested?

Three new unit tests added to tests/core/test_unit/test_classify/test_tools.py:

  • test_get_model_existing_takes_precedence — verifies load_model is called (not build_model) when both arguments are provided
  • test_get_model_builds_with_depth_only — verifies build_model is called when only network_depth is given
  • test_get_model_no_model_no_depth — verifies ValueError is raised when both are None

All 5 tests pass locally. get_model now has 100% line coverage.

Is this a breaking change?

No. The only behavioral change is for the (None, None) case, which was already broken (crashed with an unhelpful Keras error). It now raises a clear ValueError instead.

Does this PR require an update to the documentation?

No. This is an internal bug fix with no user-facing API changes.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

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.

1 participant