Skip to content
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

Type hints #79

Merged
merged 38 commits into from
Mar 5, 2020
Merged

Type hints #79

merged 38 commits into from
Mar 5, 2020

Conversation

tellezsanti
Copy link
Contributor

@tellezsanti tellezsanti commented Feb 25, 2020

In this PR

  • Added type hints, and added type checking in CI/CD test

  • Refactored the inheritance structure

             _Model
                 |-- _NonTrainableModel
                 |       |-- _NonTrainableUnivariateModel
                 |       |       |-- _NonTrainableUnivariateDetector
                 |       |       |       |-- ThresholdAD
                 |       |       |
                 |       |       |-- _NonTrainableUnivariateTransformer
                 |       |               |-- RollingAggregate
                 |       |               |-- DoubleRollingAggregate
                 |       |               |-- Retrospect
                 |       |               |-- StandardScale
                 |       |
                 |       |-- _NonTrainableMultivariateModel
                 |               |-- _NonTrainableMultivariateTransformer
                 |                       |-- SumAll
                 |
                 |-- _TrainableModel
                 |       |-- _TrainableUnivariateModel
                 |       |       |-- _TrainableUnivariateDetector
                 |       |       |       |-- QuantileAD
                 |       |       |       |-- InterQuartileRangeAD
                 |       |       |       |-- GeneralizedESDTestAD
                 |       |       |       |-- PersistAD
                 |       |       |       |-- LevelShiftAD
                 |       |       |       |-- VolatilityShiftAD
                 |       |       |       |-- SeasonalAD
                 |       |       |       |-- AutoregressionAD
                 |       |       |       |-- CustomizedDetector1D
                 |       |       |
                 |       |       |-- _TrainableUnivariateTransformer
                 |       |               |-- ClassicSeasonalDecomposition
                 |       |               |-- CustomizedTransformer1D
                 |       |
                 |       |-- _TrainableMultivariateModel
                 |               |-- _TrainableMultivariateDetector
                 |               |       |-- MinClusterDetector
                 |               |       |-- OutlierDetector
                 |               |       |-- RegressionAD
                 |               |       |-- PcaAD
                 |               |       |-- CustomizedDetectorHD
                 |               |
                 |               |-- _TrainableMultivariateTransformer
                 |                       |-- RegressionResidual
                 |                       |-- PcaProjection
                 |                       |-- PcaReconstruction
                 |                       |-- PcaReconstructionError
                 |                       |-- CustomizedTransformerHD
                 |
                 |-- _Aggregator
                         |-- AndAggregator
                         |-- OrAggregator
                         |-- CustomizedAggregator
    
    
  • We made all second-order sub-modules private and user now can only import from first-order modules, i.e.

      - adtk.detector
      - adtk.transformer
      - adtk.aggregator
      - adtk.pipe
      - adtk.data
      - adtk.metrics
      - adtk.visualization
    
  • Improved docstrings and API documentation

  • Fixed minor bugs and typos

  • Turned some parameters in some models required

    • window in adtk.detector.LevelShiftAD
    • window in adtk.detector.VolatilityShiftAD
    • window in adtk.transformer.RollingAggregate
    • window in adtk.transformer.DoubleRollingAggregate
    • model in adtk.detector.MinClusterDetector
    • model in adtk.detector.OutlierDetector
    • target and regressor in adtk.detector.RegressionAD
    • target and regressor in adtk.transformer.RegressionResidual
    • aggregate_func in adtk.aggregator.CustomizedAggregator
    • detect_func in adtk.detector.CustomizedDetector1D
    • detect_func in adtk.detector.CustomizedDetectorHD
    • transform_func in adtk.transformer.CustomizedTransformer1D
    • transform_func in adtk.detector.CustomizedTransformer1D
    • steps in adtk.pipe.Pipeline

@tailaiw
Copy link
Contributor

tailaiw commented Feb 28, 2020

@tellezsanti Thanks for the PR!

I just started to review it. A couple of quick issues I noticed before looking into the code.

  • We don't need to add type hints to unit tests, because there are few typing issues there. Can you roll back the changes?
  • I would keep docs/conf.py as it was because it is an auto-generated configuration file of Sphinx and we don't have few typing issues there either.
  • Your commit contained some temporary .DS_Store file. Please delete them. The .gitignore file in the repo is for Python only so it does not include it. You may want to add it to your own global .gitignore file.
  • Please add mypy to setup.cfg as required for testing, and add it to CI/CD.

I will continue to review the changes to code and request change if needed.

@tailaiw
Copy link
Contributor

tailaiw commented Mar 5, 2020

@tellezsanti I made some (big) changes following your works (see updated PR description). If you have no problem, I'm gonna merge.

@tailaiw tailaiw merged commit b7b2296 into develop Mar 5, 2020
tailaiw added a commit that referenced this pull request Mar 6, 2020
This reverts commit b7b2296.
@tailaiw tailaiw mentioned this pull request Mar 6, 2020
tailaiw added a commit that referenced this pull request Mar 6, 2020
@tellezsanti tellezsanti mentioned this pull request Mar 6, 2020
tailaiw added a commit that referenced this pull request Mar 6, 2020
* Added type hints to conf.py
Added mypy.ini config file for mypy

* type hints

* type hints

* mypy check

* aggragtor fix

* type hints

* corrected type hints

* clean up

* clean up

* undo unnecessary typing

* Refactor inheritance structure (#83)

* working on refactoring inheritance

* update base

* update base

* cleaned up base update

* turned on sync_params in init

* deleted old base archive

* removed Literal

* a lot more work on typing

* fixed type around pipe

* more cleaning up

* fixed all type hints in metrics

* fixed a type

* clean up

* use set_params

* comment out non-trainable multivariate detector

* removed unnecessary lines

* omit @Property and @overload

* fixed type

* allowed empty steps in pipenet

* omit plot_flowchart in coverage

* some more cleaning up

* removed unnecessary

* hide sub-modules to private

* some docstring fix

* a lot of cleaning work on docstrings

* fixed a typo

* loose callable type

* added mypy to CI/CD

* fixed docstring

* updated version number

* updated changelog

Co-authored-by: tailaiw <tailai.wen@arundo.com>
Co-authored-by: tailaiw <29800495+tailaiw@users.noreply.github.com>
@tailaiw tailaiw mentioned this pull request Mar 9, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants