Refactor model and metrics

[jasonjewik]

I'll come back to add examples later of how to use the code, and I'm really spotty on documentation, but I'm opening the PR so folks can take a look. Primary changes are such:

• Much easier to load presets (which grabs the model, optimizer, and LR scheduler [if it exists])
• Metrics are now their own subpackage
• Transforms are separated from metrics because IMO the metrics should not have to know they are transforming their inputs

[jasonjewik]

See Untitled.ipynb which can be run on MINT1 for some examples.

[jasonjewik]

TODO: Vision Transformer is broken. Up to you whether I should fix that in this PR or in a future one.

[prakhar6sharma]

TODO: Vision Transformer is broken. Up to you whether I should fix that in this PR or in a future one.

Let's leave this for a future PR.

[prakhar6sharma]

@jasonjewik thanks a lot for this crucial PR. I have done an initial review for the transforms, metrics and loaders.py and added some comments. Most of them include actionable suggestions whereas for a few I have raised the problem without going over the solution(because I couldn't think of a solution).

I am starting my review on the models/ but in the meantime, please address these comments at a time of your convenience.

[jasonjewik]

@prakhar6sharma I don't see your comments - perhaps you did not submit the review?

[jasonjewik]

Other than these changes, I feel that each model under hub/ such as Climatology, Interpolation, LinearRegression, ResNet, etc should have an attribute named in_vars and out_vars other than in_channels and out_channels.

I believe that it would allow us to implement some crucial checks such as in the case of Persistence, we need the out_vars to be subset of in_vars. Using just in_channels and out_channels it's not possible to do so. Similarly, in the case of Interpolation we can ensure that in_vars are same as out_vars.

I disagree. For clean abstraction of code, the models should not be aware of the variable names. It is the loader functions' responsibility to ensure that the requested model is legal.

[prakhar6sharma]

Added some more comments.

[prakhar6sharma]

Other than these changes, I feel that each model under hub/ such as Climatology, Interpolation, LinearRegression, ResNet, etc should have an attribute named in_vars and out_vars other than in_channels and out_channels.
I believe that it would allow us to implement some crucial checks such as in the case of Persistence, we need the out_vars to be subset of in_vars. Using just in_channels and out_channels it's not possible to do so. Similarly, in the case of Interpolation we can ensure that in_vars are same as out_vars.

I disagree. For clean abstraction of code, the models should not be aware of the variable names. It is the loader functions' responsibility to ensure that the requested model is legal.

Do we assume that the user would create the models using the loader functions only?

• If yes, then I completely agree with you.
• If not, then we are allowing silent bugs to happen.
  -- Let me give an example of Persistence, let's say the len(in_vars) is same as len(out_vars) but out_vars is not a subset of in_vars. In this case, Persistence would work fine but logically the output would be incorrect.
  -- Moreover, we should support Persistence for cases where out_vars is a subset of in_vars. Unfortunately, the current attributes of the class doesn't allow it to know which channels of input are present in output also.
  -- Similarly, I can also build an example for Interpolation.

[jasonjewik]

Do we assume that the user would create the models using the loader functions only?

• If yes, then I completely agree with you.
• If not, then we are allowing silent bugs to happen.
  -- Let me give an example of Persistence, let's say the len(in_vars) is same as len(out_vars) but out_vars is not a subset of in_vars. In this case, Persistence would work fine but logically the output would be incorrect.
  -- Moreover, we should support Persistence for cases where out_vars is a subset of in_vars. Unfortunately, the current attributes of the class doesn't allow it to know which channels of input are present in output also.
  -- Similarly, I can also build an example for Interpolation.

Yes. I assume the user to create models with the loader function only. Side-by-side comparison of the number of lines of code required, assuming the data module dm has already been created, setup, etc. shows that it's the same.

# using the loaders
import climate_learn as cl
model, optimizer, lr_scheduler = cl.load_preset("forecasting", dm, "persistence")

# do it yourself
from climate_learn.models.hub import Persistence
model = Persistence()

Besides, as the user wishes to load more models, the first approach scales better since they don't have to add a new import for each model. If the user chooses to do the second route (load it manually) and something like what you describe happens, IMO it is the user's fault, not the code's fault. Happy to discuss further.

[prakhar6sharma]

Yes. I assume the user to create models with the loader function only. Side-by-side comparison of the number of lines of code required, assuming the data module dm has already been created, setup, etc. shows that it's the same.

# using the loaders
import climate_learn as cl
model, optimizer, lr_scheduler = cl.load_preset("forecasting", dm, "persistence")

# do it yourself
from climate_learn.models.hub import Persistence
model = Persistence()

Besides, as the user wishes to load more models, the first approach scales better since they don't have to add a new import for each model. If the user chooses to do the second route (load it manually) and something like what you describe happens, IMO it is the user's fault, not the code's fault. Happy to discuss further.

Looks good to me. It's just that the loaders.py would need to be changed every time you add a model.

Also, can you please add support for Persistence for cases where out_vars is a subset of in_vars.

[prakhar6sharma]

Hopefully, this one should be the last request change.

[prakhar6sharma]

Hopefully, this one should be the last request change.

[prakhar6sharma]

Looks good to me!!!