Comments (6)
It probably makes the most sense to follow PEP-8 standards - https://www.python.org/dev/peps/pep-0008/
I believe that you can add a PEP-8 linter to your pull request process.
https://gist.github.com/MichaelCurrie/802ce28c993ff2dd632c
I really like how numpy documents their code. Here is an example and documentation.
https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html
https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard
from matrixprofile-ts.
@tylerwmarrs that's a fair point. Do you have any specific proposals, or issues that really stand out?
from matrixprofile-ts.
I agree - the code for SCRIMP++ is especially hard to follow.
from matrixprofile-ts.
@ameya98 SCRIMP++ is written with the goal to have almost every piece of the code unit tested. The fact of the matter is that SCRIMP++ is more complex than the original algorithms. Do you have any suggestions on how to improve the module so that it is easier to follow?
from matrixprofile-ts.
Obviously, I do not intend to criticize this library and your work, because I think it is a great contribution for everyone to use. However, if you do want constructive feedback:
- Why are there so many functions that seem to do extremely simple operations? Unit-testing does not require breaking extremely simple operations into multiple functions. You can check whatever properties you want to check inside your tests - there's no need for the source code to be so convoluted.
- Variable names such as tmp_a, tmp_b and so on - this makes the code absolutely unmaintainable.
- Most of these functions have 5+ parameters - this is not recommended at all as it makes the function too verbose and harder to test.
Use a static analysis tool (eg. Codebeat) that will give your code a rating and tell you where/how to improve the code by refactoring.
from matrixprofile-ts.
@ameya98 I very much so appreciate your feedback. All criticism is taken in the right context. I am probably a little bias towards the structure of the module as I wrote it. I think that it is still maintainable as is, but for others to follow, it definitely could use some refactoring.
Extremely simple operations - this shows the slow approach to how I implemented the algorithm. Literally one major computation at a time. This allowed for me to leave the algorithm and come back to it easily.
The variable names are not descriptive enough in some cases for sure.
I hope you can find some time to give this module the appropriate refactoring it needs. Unfortunately, I have other pressing issues at hand that will prevent me from doing this for a while.
As an aside, have you looked at the Matlab implementations of these algorithms? I would argue that those are difficult to follow š. Instead of hijacking this thread for SCRIMP++, please create a new issue if you are able to refactor.
from matrixprofile-ts.
Related Issues (20)
- import error
- ā ļø Archival Notice HOT 1
- stumpy HOT 2
- Definition and explanation of parameters HOT 7
- Exclusion Zone
- Return actual distances from MASS instead of squared distances HOT 2
- motifs.motifs throws exceptions on larger arrays HOT 1
- Question: Finding Shared Motifs for Clustering HOT 2
- O(nĀ²) Memory Requirements for _stamp_parallel()
- Complexity Measure confuses mean and sum HOT 1
- Duplicate license files: remove License.md? HOT 4
- How to handle NaN and INF values in utils.py movmeanstd and movstd
- Stomp increment is missing HOT 2
- MPdist
- matrixProfile.stomp() gives nan and inf values HOT 5
- Annotation Vector file missing from latest matrixprofile-ts build HOT 3
- ValueError: Length of values does not match length of index HOT 1
- Pattern Recognition HOT 6
- What is the difference between matrixprofile and matrixprofile-ts? HOT 8
- Use of 'is' for non-object. HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
š Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ššš
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ā¤ļø Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from matrixprofile-ts.