[ENH] Improve deep learning networks test coverage for parameters which can be list

[Cyril-Meyer]

Reference Issues/PRs

Improves part of #1233

What does this implement/fix? Explain your changes.

Improve the test_all_networks_functionality tests, adding cases when some parameters can be a list.

Before	After

Does your contribution introduce a new dependency? If yes, which one?

No.

Any other comments?

Some tests won't work and are disabled as 'continue exceptions'. I don't know if this is the right way to do it.
In any case, these problems will need to be fixed, and are related to the implementations themselves.
Anyway, we are adding new tests, and these cases are just like before, without the improvement.

# Exceptions to fix
if (
    attrname in ["kernel_size", "padding"]
    and network.__name__ == "TapNetNetwork"
):
    continue
if attrname in ["activation"] and network.__name__ == "AEBiGRUNetwork":
    continue
# LITENetwork does not seem to work with list args
if network.__name__ == "LITENetwork":
    continue

PR checklist

For all contributions

• I've added myself to the list of contributors. Alternatively, you can use the @all-contributors bot to do this for you.
• The PR title starts with either [ENH], [MNT], [DOC], [BUG], [REF], [DEP] or [GOV] indicating whether the PR topic is related to enhancement, maintenance, documentation, bugs, refactoring, deprecation or governance.

[aeon-actions-bot]

Thank you for contributing to aeon

I have added the following labels to this PR based on the title: [ $\color{#FEF1BE}{\textsf{enhancement}}$ ].
I have added the following labels to this PR based on the changes made: [ $\color{#379E11}{\textsf{networks}}$ ]. Feel free to change these if they do not properly represent the PR.

The Checks tab will show the status of our automated tests. You can click on individual test runs in the tab or "Details" in the panel below to see more information if there is a failure.

If our pre-commit code quality check fails, any trivial fixes will automatically be pushed to your PR unless it is a draft.

Don't hesitate to ask questions on the aeon Slack channel if you have any.

PR CI actions

These checkboxes will add labels to enable/disable CI functionality for this PR. This may not take effect immediately, and a new commit may be required to run the new configuration.

• Run pre-commit checks for all files
• Run all pytest tests and configurations
• Run all notebook example tests
• Run numba-disabled codecov tests
• Stop automatic pre-commit fixes (always disabled for drafts)
• Push an empty commit to re-run CI checks

[hadifawaz1999]

Thanks for taking care of this, coverage seems great now !

We should open an issue after this getting merged for cases like LITE (still not parametrized as it should with list inputs)

[MatthewMiddlehurst]

This should be a separate test IMO i.e. test_all_networks_params.

Will these parameters be in every network (even future ones)? May be missing something, but would this fail if a network does not have one from the list?

[hadifawaz1999]

This should be a separate test IMO i.e. test_all_networks_functionality.

Will these parameters be in every network (even future ones)? May be missing something, but would this fail if a network does not have one from the list?

hmm good point @MatthewMiddlehurst , maybe we can have a test per network ? test_inception_parameters, test_lite_parameters etc.

[Cyril-Meyer]

This should be a separate test IMO i.e. test_all_networks_params.

Not sure about this as it's exactly the same test as before but with different types for some parameters.
But yeah, it's easy to change as you asked :)

hmm good point @MatthewMiddlehurst , maybe we can have a test per network ? test_inception_parameters, test_lite_parameters etc.

This is also a valid choice, but which requires a new test to be created for each new network.
For now, I was mimicking the actual tests that do not require that.

Will these parameters be in every network (even future ones)? May be missing something, but would this fail if a network does not have one from the list?

The networks do not share the parameters, that's why we check that the network have the parameters or not. So no, this would not fail if a network does not have the parameter.
The only reason to have a fail is if a new network use the same parameters name but do not allow the usage of both types (e.g. like LITE for now).

[MatthewMiddlehurst]

I do not mind it being a singular test for now instead of going through each file and adding a bunch of tests. Ideally the parameters would be used in more than one network though.

Not sure about this as it's exactly the same test as before but with different types for some parameters.
But yeah, it's easy to change as you asked :)

I think it's acceptable to lose a bit of testing efficiency here for readability. You don't have to assert the output etc like the current test. I would like to avoid forming the "omni-test" for all networks which just covers everything in a single function, it was kinda like that before 🙂.

[Cyril-Meyer]

I do not mind it being a singular test for now instead of going through each file and adding a bunch of tests. Ideally the parameters would be used in more than one network though.
I think it's acceptable to lose a bit of testing efficiency here for readability. You don't have to assert the output etc like the current test. I would like to avoid forming the "omni-test" for all networks which just covers everything in a single function, it was kinda like that before 🙂.

I added the new test as a new function, test_all_networks_params as proposed.
I also changed the AE skip using tag instead of the network name and added a skip info.

Also, is tensorflow-addons requirement still needed ? ref

[hadifawaz1999]

I added the new test as a new function, test_all_networks_params as proposed. I also changed the AE skip using tag instead of the network name and added a skip info.

Also, is tensorflow-addons requirement still needed ? ref

thanks for the changes @Cyril-Meyer ! the check for config existence is good solution for now, we should keep in mind removing it once we add the config to all other networks

great catch on tensorflow-addons, we dont need it no, we removed its usage before and project no longer depends on it, probably forgot to remove it here

[Cyril-Meyer]

thanks for the changes @Cyril-Meyer ! the check for config existence is good solution for now, we should keep in mind removing it once we add the config to all other networks

The check was not necessary, as the base class has it :

    _config = {
        "python_dependencies": ["tensorflow"],
        "python_version": "<3.12",
        "structure": "encoder",
    }

I just didn't see this before, but it's cool.

great catch on tensorflow-addons, we dont need it no, we removed its usage before and project no longer depends on it, probably forgot to remove it here

Yeah, I think it will be more clear if we create a new issue and solve it with a new PR (I can handle that).

[hadifawaz1999]

Looks good to me @MatthewMiddlehurst how about you ? (btw for some reason readthedocs says "pending" for 12 hours now)

[MatthewMiddlehurst]

Looks fine for now. Feel free to merge when you think its good.

[MatthewMiddlehurst]

Can ignore the docs, says it has built fine when you click on details.

[hadifawaz1999]

LGTM! thanks for taking care of this