-
Notifications
You must be signed in to change notification settings - Fork 68
Convert modules into sub-packages #601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Split into `config/__init__.py` and `config/utils.py`
Split into `experiments/__init__.py`, `experiments/input.py`, `experiments/output.py`, and `experiments/utils.py`.
Split into `learner/__init__.py` and `learner/utils.py`.
Move all `utilities/*.py` into `utils/commadline/*.py`. Move skll constants from all across codebase into `utils/constants.py`. Move `logutils.py` into `utils/logging.py`.
- Remove unnecessary functions and classes from top-level namespace. - Remove some unnecessary newlines.
- Add new sections for `config` and `utils` packages. - Replace "module" with "Package" in various places. - Remove documentation for functions that are no longer in top-level skll namespace.
- Call it `load_custom_learner()` and have it return the object instead of populating `globals()` directly. - Populate globals() where this function is called instead,
|
Hello @desilinguist! Thanks for updating this PR.
Comment last updated at 2020-04-11 14:00:59 UTC |
Codecov Report
@@ Coverage Diff @@
## master #601 +/- ##
==========================================
+ Coverage 95.06% 95.16% +0.10%
==========================================
Files 20 26 +6
Lines 2977 3021 +44
==========================================
+ Hits 2830 2875 +45
+ Misses 147 146 -1
Continue to review full report at Codecov.
|
mulhod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I think this was really needed. It was getting difficult to develop in the long modules.
aoifecahill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
Co-Authored-By: Matt Mulholland <mulhodm@gmail.com>
This PR closes #600.
The various SKLL modules
learner.py,experiments.py,config.pyetc. were getting really long as single files and unwieldy when it came to adding new development. This PR tries to address this issue by converting these modules into sub-packages instead with specific functions moved to different modules under these sub-package.Here's what the SKLL code tree looks like today:
With this PR, it will look like:
In addition, some functions that were previously indicated to be private (with a leading underscore) are now public since they can actually be quite useful as part of the API. Some examples include
experiments.input.load_featureset()andexperiments.output.generate_learning_curve_plots().Specifically, this PR:
__init__.pyfor these sub-packages such that most of the idiomatic SKLL imports work just like they do now.skllnamespace. Right now only the following are importable fromskll-FeatureSet,Learner, andrun_configuration.logutils.pytoutils/logging.pyutils/constants.py.utils/commandline/and adjustssetup.pyaccordingly._import_custom_learner()function to a public function calledload_custom_learner()which does not modifyglobals()directly but just returns the loaded learner to the caller which then modifiesglobals()on its end.test_logutils.pyandtest_utilities.pytotest_logging_utils.pyandtest_commandline_utils.py.