-
Notifications
You must be signed in to change notification settings - Fork 338
[WIP] Add mmotifs and aamp_mmoitfs Unit Tests
#553
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
|
@SaVoAMP Before pushing your commits, please remember to execute |
I am not sure what to do because I get many problems when running flake8 . |
Codecov Report
@@ Coverage Diff @@
## main #553 +/- ##
==========================================
+ Coverage 98.93% 99.89% +0.95%
==========================================
Files 74 80 +6
Lines 10732 11192 +460
==========================================
+ Hits 10618 11180 +562
+ Misses 114 12 -102
Continue to review full report at Codecov.
|
|
@SaVoAMP Can you try running those commands in the root STUMPY directory instead on inside the There is a file called |
|
Ohh, sorry, that was pretty stupid! I probably should not make pull requests while having a headache 😆 |
|
I tried to write the first testing function. I'm not sure whether I did it as expected 🤔 Never mind, I see the problem 😆 |
|
I think we have got a problem with I think we need something like I have got You don't have to look at the |
Based on |
|
I get exit code 5 on pytest which means, there are no tests to run. I don't see any assertions (using the assert keyword) as seen in the documentation of pytest in your code. I tried to use your code but I still get the same error message. What am I missing? |
Based on the order of the code, I see that So, with
Also, please do not feel the need to apologize. I get confused by this stuff as well and we are all here to learn/help/exchange ideas. Open source is a team sport and we should support each other! |
Let me take a look. |
I think I understand what the issue is after looking at the unit tests that are being executed here: This is consistent with what you are observing. However, it wasn't clear which test file was experiencing an Okay, so the next test is
Because there are, indeed, no tests to run inside of Then you should be able to overcome this and then doing On a side note, I do see that all 4 tests for Now, to address your comment:
Yes, it is true that I don't explicitly use the Notice that we have a reference In fact, you can combine both sets of Please let me know if I can help fill any knowledge/understanding gaps. |
|
Awesome! It looks like all tests are passing now (even the dummy one). Now, you can focus on writing the tests for coverage: Please ignore the missing coverage for |
|
Alright, perfect! Would you mind taking a look at my first test function in |
Yes, please give me some time to review |
tests/test_mmotifs.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize("Q, T", test_data) | ||
| def test_match(Q, T): |
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.
Since match is technically in the motifs.py module, I feel like this should get moved into the tests/test_motifs.py test file
tests/test_mmotifs.py
Outdated
| def test_motifs_multidimensional_one_motif_all_dimensions(): | ||
| # Find the two dimensional motif pair | ||
|
|
||
| # Arrange |
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.
Can you please tell me how you came up with this set of 4 arrays? Did you determine them all by hand (i.e., without invoking stumpy.mmotifs)?
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.
I was using already covered functions to obtain them. Is this ok?
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.
Yes, that should be fine. However, if for some reason those functions are not covered then we should also cover them first.
tests/test_mmotifs.py
Outdated
| T, P, I, max_distance=np.inf, max_matches=2, k=1 | ||
| ) | ||
|
|
||
| # Assert |
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.
Yep, if the tests pass then you are on the right track! The goal here is to compare your manual calculations (without calling the mmotifs() function) with what the mmotifs() function would produce.
Then, you try to test all possible combinations of inputs by hand (note your expected manual outputs) and then see if the function you are testing also produces the same expected outputs. If not then there is something wrong with your function.
After that, the goal is to think about nasty edge cases and to write tests to ensure that we are handling those edge cases properly. We don't have to catch everything (it's impossible to) but at least think about how a user might use/abuse the function and hopefully we test and make sure that we consider all reasonable (i.e., rationally obvious) use cases and have good coverage on those.
Also, as you may have noticed, the goal of having a "naive" implementation means that you don't have to do things manually and, instead, it allows you to try random inputs of different small arrays (in addition to a pre-determined array with known outputs). The only difference between the naive implementation vs your implementation should be speed (because the naive implementation would use a bunch of slow for-loops but is easier to read and follow versus faster algorithms are usually VERY HARD to read/follow since they often employ hyper specialized tricks).
@SaVoAMP I've left a few comments but, seriously, (no surprise) you're doing a great job! You'll be teaching me about unit testing soon :) |
|
@SaVoAMP Can you please provide me with the set of input parameters for |
|
@SaVoAMP In this rare case, please add I don't usually recommend this but this is an exceptional case that requires it. |
It looks as if everything is covered now. |
Awesome! Maybe you can focus on the tutorial now and I will find some time to thoroughly review the tests and code changes. Thank you for all of your efforts! |
seanlaw
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.
@SaVoAMP I think things look pretty good. I provided some comments for you to consider. All comments for test_aamp_mmoitfs.py should also apply to test_mmotifs.py. Please, let me know if you have any questions, comments, or concerns.
|
@SaVoAMP Thank you for putting in all of this work. I hope that I'm not coming across as too "nit-picky" (I probably am 😄). However, since you had expressed that you are relatively new to (some?) of this software development process, I wanted to at least provide the appropriate feedback and "thinking" regarding best practices. While I could have accepted your PR "as is", I didn't want to take away your opportunity to learn, try, and gain more "real world" exposure on the topic. As you can see, this part is often more art than science but I'm sure you can understand why it takes me a long time (working by myself) to release new features as I care deeply about maintaining the high quality of that last Kilometer. Things like unit testing and documentation are often the hardest part of the development process that gets heavily neglected because it isn't glamorous work (like adding a new feature) and you've really excelled! However, unit testing and documentation are an integral (if not the most important) part of open source software so please know that you are really helping STUMPY uphold its standards as well as its commitment to its community! Thank you and I appreciate your patience |
seanlaw
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.
@SaVoAMP We are very, very close to having your PR merged! Just a few minor comments and suggestions for you to consider.
| ] | ||
|
|
||
|
|
||
| def test_aamp_mmotifs_default_parameters(): |
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.
Perfect!
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.
I don't know if you notice this but after we take out the chunk of code for T, the test function(s) are all super simple and it is easy to digest what is happening. This little bit of refactoring, cleans up the code and everything can be read without scrolling. This is exactly how a "good" unit test should look and this is about as close to "perfection" as I could expect, which makes my job as a reviewer really, really easy. The rest of the unit tests look equally as great too! Excellent work!
"Future Sean" thanks you as well. 😄
|
Thank you for your support!
Yes, as I've already mentioned these are my first pull requests ever. I started my major without any prior experience with computers (I didn't even had one and bought my first computer ever in the third semester 😆) as my specialization in "high school" was social work. And although my current major included some computer science modules (C# Programming, data structures and algorithms, very basic software engineering without git) the focus is still on Math and Physics, we are only required to do those modules to be able to do numerical simulations with tools like COMSOL Multiphysics and the like. Since the start of my mandatory apprenticeship and successive bachelors thesis at the Fraunhofer Institute I'm working on data science projects and had to learn Python, Linux and git from scratch over the last months. Therefore I especially value your professional and detailed feedback. (I also brush up my english skills along the way 😄) |
😮 🤯 I hope you realize that you are well on your way in your professional career! You've picked things up really, really quickly and, if this is your first time, I can only imagine how successful you will be when all of this becomes second nature to you. 😄 I've given you only a bit of remote guidance and you've been resourceful and proactive. It's been a pleasure working with you!
I was actually thinking about this the other day. Your English is excellent and it is not lost on me that you are needing to push through the language barrier in addition to learning brand new software development skills as well as balancing your responsibilities as a student. Really amazing! I sincerely appreciate your dedication. I hope that this isn't a huge distraction/stress for you. |
|
Though I must admit, I can't resist the temptation to use the DeepL translator a little too often 😄 I'm jokingly referring to this since I was surprised about you answering my questions on weekends. 😆 |
I completely understand. STUMPY is a labor of love and 100% volunteer and I can't expect other people to care more about it than I do. Having said that, my unsolicited advice is "don't be like me" 😅 . As a former academic (scientific researcher), I have accrued some really bad work habits and you should not use me as a "good example". I am lucky that I love STUMPY and so I don't feel that this is "work". But you must find your balance and figure out what is important to you and what you are passionate about. I am grateful for people like you who wish to contribute as I cannot do all of this myself! |
|
@SaVoAMP Not trying to rush you but will you let me know when it is appropriate for me to review this PR again? |
|
Ohh, I'm sorry! I was just about to ask you the same question 😆 |
|
Great! Glad I asked. I will find some time to review it |
|
@SaVoAMP Congratulations on another wonderful PR being merged. Thank you for this wonderful contribution! |


Pull Request Checklist
Below is a simple checklist but please do not hesitate to ask for assistance!
black(i.e.,python -m pip install blackorconda install -c conda-forge black)flake8(i.e.,python -m pip install flake8orconda install -c conda-forge flake8)pytest-cov(i.e.,python -m pip install pytest-covorconda install -c conda-forge pytest-cov)black .in the root stumpy directoryflake8 .in the root stumpy directory./setup.sh && ./test.shin the root stumpy directoryAdd Unit Tests for
mmotifsandaamp_mmotifs#552