Skip to content

Added validate_input_sizes function to Model class - #97

Merged
ixjlyons merged 6 commits into
nasa:mainfrom
tarplmic:add_input_size_check_to_model
Sep 30, 2026
Merged

ixjlyons merged 6 commits into
nasa:mainfrom
tarplmic:add_input_size_check_to_model

Conversation

@tarplmic

@tarplmic tarplmic commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Added a validate_input_sizes function to the Model class. This loops through every input_field for the class (on init) and checks that the size of each input is the same for the class definition and the instance of that class. If there are differences, it tells the user what the name and type of the input is, the desired shape, and input shape.

@sixpearls

Copy link
Copy Markdown
Contributor

Can you add a test that shows it is catching an invalid input? pytest has a context manager for an expected exception.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This breaks tuple inputs that used to work. flatten_value() already accepts generic array-like values by converting non-arrays with np.array(), but the new validator only normalizes int/float/ndarray/list. A vector passed as (5.0, 2.0, 1.0) now reaches input_value.shape as a tuple and raises AttributeError before the existing conversion. Could this use np.asarray/generic array-like handling and add a tuple regression?

@sixpearls

Copy link
Copy Markdown
Contributor

Good catch on the tuple. It may work to change the condition isinstance(input_value, (int, float, np.ndarray, list)) to not isinstance(input_value, backend.symbol_class)

@sylvesterkaczmarek

Copy link
Copy Markdown

Yes, I think using the symbolic versus non-symbolic boundary is cleaner here. flatten_value already treats anything that is not a backend symbol as array-like by converting it through NumPy, so doing the same before shape validation would keep tuples and other numeric array-likes compatible without maintaining a separate hard-coded type list. I would still add the tuple regression from my review, plus the invalid-size exception test you requested, so both compatibility and the new validation behavior are pinned.

…puts as well. Added test_invalid_size_check to ensure the validate_input_sizes function is working correctly.

@sixpearls sixpearls left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. After tests pass, @ixjlyons can merge if he approves

Comment thread tests/test_model_api.py Outdated

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked current 03d2f01a. The tuple compatibility issue I raised is resolved: non-symbolic values now go through np.atleast_2d before shape validation, so tuple/list/ndarray inputs share the same normalization path instead of tuple values reaching .shape directly.

The new test covers tuple success, ndarray/backend-symbol success, and invalid tuple/array/symbol shapes. The full CI matrix is green across supported Python versions and platforms. No remaining issue from my earlier review; the current maintainer comments are test-organization/assertion refinements rather than correctness blockers.

@ixjlyons
ixjlyons merged commit a182017 into nasa:main Sep 30, 2026
34 checks passed
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.

5 participants