Skip to content

[BUG] Fix GreedyEncoder max_len type hint and add tests (#376)#378

Open
sohamjadhav95 wants to merge 1 commit intogc-os-ai:mainfrom
sohamjadhav95:fix/greedy-encoder-test-params
Open

[BUG] Fix GreedyEncoder max_len type hint and add tests (#376)#378
sohamjadhav95 wants to merge 1 commit intogc-os-ai:mainfrom
sohamjadhav95:fix/greedy-encoder-test-params

Conversation

@sohamjadhav95
Copy link
Copy Markdown

Reference Issues/PRs

Fixes #376

What does this implement/fix? Explain your changes.

GreedyEncoder.__init__ had max_len defined as a required int argument with no default, but the docstring describes it as optional with a default of None, and _transform already has handling for the None case. This mismatch meant get_test_params() was broken — param0 doesn't include max_len, so trying to instantiate GreedyEncoder(**param0) raised a TypeError.

Fix is a one-line change: max_len: intmax_len: int | None = None, making the signature consistent with the existing docstring and implementation.

What should a reviewer concentrate their feedback on?

  • The type hint change in __init__ — making sure int | None = None is the right approach vs adding max_len to param0 instead
  • The 5 new tests covering: get_test_params instantiation, max_len=None default behavior, truncation, padding, and unknown token handling

Did you add any tests for the change?

Yes. Created pyaptamer/trafos/encode/tests/test_greedy_encoder.py from scratch — no tests existed for GreedyEncoder before this PR. Added 5 tests covering the main behaviors.

Any other comments?

Found this while going through the trafos module. The GreedyEncoder had no test file at all, so added coverage for the core functionality alongside the fix.

PR checklist

  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG].
  • Added/modified tests
  • Used pre-commit hooks when committing to ensure that code is compliant with hooks.

@sohamjadhav95
Copy link
Copy Markdown
Author

@NennoMP @siddharth7113 PR #378 is ready for review.

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.

[BUG] GreedyEncoder.get_test_params() missing required max_len in param0

1 participant