Fix mutable defaults - Githubissues

MIDASverse / MIDASpy

Python package for missing-data imputation with deep learning

Apache License 2.0

125 stars 35 forks source link

Fix mutable defaults #24

Closed David-Woroniuk closed 1 year ago

David-Woroniuk commented 2 years ago

I have made a few changes as it relates to the sanity and typechecking of:

layer_structure output_layers weight_decay output_structure seed

Additionally,

_batch_iter_zsample and _sort_cols appear to work as staticmethods, I have applied the decorator.

Please test ahead of merging, as this should not make any breaking changes.

tsrobinson commented 1 year ago

Thanks @David-Woroniuk, this is all really helpful and I appreciate it greatly. I'll check and then merge today!

David-Woroniuk commented 1 year ago

Thanks @tsrobinson - a few points to mention on the code base:

mutable function defaults is generally considered an antipattern (https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/)
Static methods can neither modify object state nor class state - they're generally restricted in what data they can access - and they’re primarily a way to namespace methods.
Sanity & typechecks could be quite useful, especially when trying to force user behaviours.
Type hints are quite useful for other devs to understand what the required inputs are for a given function / class.

Happy to send a few more PRs for the rest of the script.

tsrobinson commented 1 year ago

This is all super helpful, and noted! Can I check whether you took a copy of the source code from the MIDASpy directory, or worked from the existing file in the build directory?

I ask because this highlights bad practise on our part since the build folder is a relic, and the up to date source code is in MIDASpy. There are a few more recent bug fixes that won't be in the build directory version.

Let me know, and I'll work out if I need to stitch your changes into the other source code file.

David-Woroniuk commented 1 year ago

This was from the build directory - happy to make the changes in a different PR and we can close this without merging? If it helps, I can make a few more commits in a separate PR and we can merge that?

tsrobinson commented 1 year ago

If you would like to, that would be fantastic. I'm sorry not to have pointed out the proper file location before. Look forward to seeing the new PRs!