Add type hinting

[ggalloni]

This is the first attempt to add type hinting throughout the repo, mentioned in #189.

I was quite generic on many occasions as this is just supposed to give us a place to work on this and can be refined once we agree on some strategy.

[cmbant]

The most useful place to have type hints is annotations for variables that are only defined in the yaml (so consistency can then be checked just using standard editors like vs code/Pycharm). In many places types can be inferred accurately, and little to be gained by adding them explicitly unless useful for doc of public interface.

Note: want to make sure you don't statically import anything that's an optional dependence (e.g. ccl).

[ggalloni]

I added checks for the quantities from yaml files, which now must match the expected types. Let me know what you think of the current strategy.

For the explicit typing in the code, I am a fan of a bit of redundancy in this case, but it is just a matter of taste. I think it makes the code more readable as the user knows, at first sight, what type a certain quantity is. Still, I am planning to go through the modifications and clean the code, because some may be too obvious.

[cmbant]

There's a CobayaComponent-level method intended for parameter validation, validate_info (https://github.com/CobayaSampler/cobaya/blob/master/cobaya/component.py#L415). You could override this in one of SOLikeT's base classes to check that all assignments match the types in the annotations. (It shouldn't really be necessary to repeat the type once defined in the attribute's annotation).

But perhaps better would be to add a general "_enforces_types" class attribute to Cobaya, and add code to Cobaya's Component.validate_info to fully validate types for any descendant class that sets _enforce_types is True.
I'm not sure if there's a built-in python function to validate a variable against a type, but Sonnet suggests using typeguard (from typeguard import check_type) or something like this

def validate_attribute(cls, name, value):
    hints = get_type_hints(cls)
    if name in hints:
        expected_type = hints[name]
        if hasattr(expected_type, '__origin__'):
            if expected_type.__origin__ is Union:
                if not any(isinstance(value, t) for t in expected_type.__args__):
                    raise TypeError(f"Attribute '{name}' must be one of types {expected_type.__args__}, not {type(value)}")
            elif expected_type.__origin__ is Optional:
                if value is not None and not isinstance(value, expected_type.__args__[0]):
                    raise TypeError(f"Attribute '{name}' must be of type {expected_type.__args__[0]} or None, not {type(value)}")
        elif not isinstance(value, expected_type):
            raise TypeError(f"Attribute '{name}' must be of type {expected_type}, not {type(value)}")

[Note I am travelling most of the next two weeks, so likely not responding to any Cobaya issues]

[ggalloni]

Starting from the code you suggested, I refactored the type-checking functions to avoid repeating the types. Also, now they accept essentially all types, including nested ones (e.g. List[str], Dict[str, float], Tuple[str, str, int], etc).

As a testing ground, I used the Bias class. Still, eventually, something similar can be moved within Cobaya and substitute validate_info (the method _validate_attribute in Bias is essentially equivalent, but allows for all types).

[ggalloni]

Hello @cmbant, now that CCL is working, all tests are OK with this.
Only Python 3.9 needed an adjustment as typing_extensions needs to be there.

[cmbant]

Sounds good, I guess could be merged once a new cobaya release.
Btw, I don't think you need provider: Provider, should be inherited.

[itrharrison]

(this is a teachable moment for me) is there a reason for this? I have always preferred to return a variable rather than an expression as I find it much easier to debug (i.e. to place break points).

[ggalloni]

As we said in the call, this is just a matter of convention I guess, so I don't have a strong opinion here

[itrharrison]

Is this whole function necessary? I would be in favour of removing it and letting people plot their own covmats. At the moment it adds an extra, fairly well hidden, dependency.

[ggalloni]

Yes, I agree. I changed this to use simple matplotlib, which will be installed anyway one way or the other. But even removing this is OK with me.

[ggalloni]

@itrharrison @mgerbino In principle this is now ready to be merged :D

The new release of cobaya is working fine. Also, I removed that hidden dependency on holoviews in favor of straightforward matplotlib. I tested on the fly on the test_gaussian.py and it is correctly plotting a covariance. No idea what holoviews would have plotted honestly (couldn't make it work), but as Ian said, I think it should not be our job to find the best way to do that.

The remaining open question of whether to return a variable or an expression is pure convention, so it is the same for me. Anyway, this is not a show-stopper.

[mgerbino]

ciao @ggalloni , yes, I agree with Ian that returning a variable is easier for debugging purposes. If it doesn't take too much time to you, could you please make the change requested by @itrharrison , so that we can merge this with master and close the issue? Thank you!

[ggalloni]

Ciao @mgerbino, I reverted that change in cluster.py, however, there are quite a lot of points in the code that return expressions.
Making this a general rule will take quite some time to fix.

Maybe we can try to return variables only when dealing with complex objects (it doesn't make much sense to do it for trivial stuff). What do you think? Still, this will take some time to track down all of these.