aeon
aeon copied to clipboard
aeon-toolkit
[DOC] Add type hints for similarity search
Describe the feature or idea you want to propose
Other modules are increasingly using type hints in the function declaration, the similarity search module should do the same.
Describe your proposed solution
Implement type hint in function declaration similarly to as it's done in other modules.
Describe alternatives you've considered, if relevant
No response
Additional context
No response
I found this on a website that aggregates good first issues. I am quite inexperienced but I have fixed some tests in pandas for example, and I know what type hints are and I think that I can get this done.
It seems that it is already done in a few places in the similarity_search directory but in other places no.
Are multiple people allowed on this? I was looking into it already, and didn't know about the aeon-assign bot. If preferred not, that's fine.
Hey @Datadote, it is considered good practice to assign yourself the issue or use the bot to avoid people working on the same stuff. If you want I could create a similar issue for another part of aeon and assign it to you ?
Hi @phershbe, there is indeed some function already with type hints, but not all. There is some of typos and mistakes in the current docstrings but I'm working on them on a seperate issue, so dont worry about it too much. If you need any help don't hesistate to ask here or on Slack !
Hey @Datadote, it is considered good practice to assign yourself the issue or use the bot to avoid people working on the same stuff. If you want I could create a similar issue for another part of aeon and assign it to you ?
Sure, that sounds good to me. Thank you.
@baraline I put in a draft pull request with two questions at the top.
pre-commit didn't pass because of two lines which were marked as too long by flake8, I was using the form of X : np.ndarray, 2D array of shape (x, y) and X : np.ndarray, 3D array of shape (x, y, z) as suggested by @TonyBagnall on Slack. They can be a little bit long but I think that they are very clear now. Of course they could be made into two lines but I tried and it made them kind of confusing. I am assuming that you can tell it to disregard these two errors, or I can change those lines, whatever you guys think is best.
Additionally, regarding this issue, there is a similarity search subdirectory for distance profiles with several files in it, I am happy to do those too if you would like, but I thought that I would do the two files in the main directory to begin with and get feedback.
If a line is too long but it is justified, you can add the #noqa XXX with XXX being the error code from precommit, the line will be skipped for this error
For your questions, i think the iterable optional is fine! For retunt self, you hint it by the class self represents.
I'll try to get the review done this weekend, no problem for the distance profiles, just create an issue an tag me if you need me to assign it to you!
Awesome, I am learning a lot. I added the type hint, commented out the two lines with the error code, and updated the description of the pull request, so it should be ready. I am in no hurry to get the review this weekend unless you want to anyway, thank you for taking the time to comment quickly on my concerns.
For the distance profiles, I think that I will do them at the start of the week and I suppose that the issue here could be kept open and I could add the pull request here if that sounds okay.
I got the other pull request in, for the distance profiles, which should complete type hinting for all of similarity search.
I am having trouble with pre-commit, long story, and I can figure it out I am sure but for now I am going to let it do it here and then it will tell me that a few lines are too long for flake8 and then I will mark them to be skipped for error.
This pull request is a little bit more complicated than the one before because this one has a lot of parameters with Union[np.ndarray, List] where the variables can come from numpy or numba, so you can check them, but I am pretty confident that I was making sense of them correctly.