Refactor data module (except iterdatamodule)

aditya-grover / climate-learn

Source code for ClimateLearn

MIT License

310 stars 49 forks source link

Refactor data module (except iterdatamodule) #68

Closed prakhar6sharma closed 1 year ago

prakhar6sharma commented 1 year ago

I have been working on this refactoring for some time now, specifically addressing the #43. It's not yet complete but opening it as a draft PR.

I have been trying to abstract out tasks, data and datamodule to decouple them from each other.

prakhar6sharma commented 1 year ago

Tasks left to do before requesting for review:

[x] Add type hints for all the classes and functions under climate_learn.data_module
[x] Create tests directory along with basic tests for all the classes under climate_learn.data_module
[x] Modify the doc notebooks to reflect changes

prakhar6sharma commented 1 year ago

I plan to do a follow-up PR (coming week) that will unify the new DataModule and @tung-nd's IterDataModule. Because of which I may need to change the interface of TaskArgs, Task, DataArgs, and Data.

As a result, I would need to rewrite doc strings if I write them in this PR. So, it would be really great if you could ignore the absence of doc strings for this PR.

prakhar6sharma commented 1 year ago

Thanks @jasonjewik for the insightful comments. Overall, I agree with many of the changes that you suggested:

[x] Changing 'Train' to 'train' and adding some sort of assertion for the split to be one of ['train', 'val', 'test']
[x] Renaming the class name from Data to ClimateDataset
[x] Renaming the folder name from data_module back to data
[x] Making DataArgs, TaskArgs, DataModuleArgs, Data inherit from abc.ABC

But I would still like to contest about the use of Args class. See my previous comments for more about this. Finally, as a user of the library, I would prefer simplicity but not if it impedes my experience as a developer. To make it easy for anyone to use, we already have the notebooks.

Also regarding type hinting is it possible to leave them there for now as it anyways doesn't affect the behavior of the library for now. I spent so much time trying to figure them out :(

prakhar6sharma commented 1 year ago

I am sorry that I had to create this massive PR but I couldn't come up with a shorter PR that incorporate structural changes without breaking the main.

jasonjewik commented 1 year ago

Thanks for addressing my comments.

Also regarding type hinting is it possible to leave them there for now as it anyways doesn't affect the behavior of the library for now. I spent so much time trying to figure them out :(

Sure, let's leave them in for now.

I am sorry that I had to create this massive PR but I couldn't come up with a shorter PR that incorporate structural changes without breaking the main.

Understandable, I just don't want to set a precedent of massive PRs.

prakhar6sharma commented 1 year ago

I think all the comments are addressed now and this PR is finally ready to be merged. 🚀