Imlemented set_config and added test cases

[FirdausChoudhury]

Implemented set_config and added test cases.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[FirdausChoudhury]

For a case where no arguments are passed into set_config, do we want it to return default values or raise an error?

[BirkhoffG]

Line #15.        if rng_reserve_size is not None:

Here, you are basically assiging values to config. Write it as a separate function so that we can reuse it for assigning global_seed.

In addition, let's not check typing and value for now. It will raise error anyway. A better way of type checking is to refactor Config class using pydantic. See DataModuleConfig as an example. I don't want you to refactor Config in this PR.

Reply via ReviewNB

[FirdausChoudhury]

Completed

[BirkhoffG]

Line #29.        for k, v in kwargs.items():

Delete this entire for-loop. First, rng_reserve_size and global_seed will not appear in kwargs (python will prevent it at compile time). Second, passing kwargs is mostly for backward compatibility. Say in the future version, we have other config attributes. We can run the code for the future version in the current version of library.

Reply via ReviewNB

[FirdausChoudhury]

Deleted the for loop

[BirkhoffG]

Line #11.    test_fail(set_config, kwargs={'rng_reserve_size': -1}, contains='must be non-negative')

Adjust test cases accordingly. Test cases before this line looks good.

Reply via ReviewNB

[BirkhoffG]

You should probably change your fourth test cases. I think we shouldn't change the behavior of Config if nothing is passed to set_config.

So the test case should be

assert get_config().rng_reserve_size == 2 and get_config().global_seed == 234

[FirdausChoudhury]

I have changed the implementation so that if a user passes no arguments to set config, then no change occurs and the previous values are still valid.

[BirkhoffG]

Please address these comments. Also, before you commit, you should run nbdev_export & nbdev_clean everytime.

[BirkhoffG]

For a case where no arguments are passed into set_config, do we want it to return default values or raise an error?

I think if you pass nothing, you should not change main_config (not simply returning default values).

[BirkhoffG]

Line #8.        

Write documentation for this function. See jax-relax - Contribute (birkhoffg.github.io)

Reply via ReviewNB

[BirkhoffG]

I would write the function as this:

def set_val(arg_name: str, arg_value: int, arg_min: int) -> None:
        """Checks the validity of the argument and returns the argument value."""

        if arg_value is None or not hasattr(main_config, arg_name):
            return 

        if not isinstance(arg_value, int):
            raise TypeError(f"`{arg_name}` must be an integer, but got {type(arg_value).__name__}.")
        if arg_value < arg_min:
            raise ValueError(f"`{arg_name}` must be non-negative, but got {arg_value}.")
        setattr(main_config, arg_name, arg_value)

set_val('rng_reserve_size', rng_reserve_size, 1)
set_val('global_seed', global_seed, 0)

[BirkhoffG]

Please fix the indent issue. Looks good to me for the rest of changes.

[BirkhoffG]

Can you fix the indent for the arguments? It should be exactly one tab.