Skip to content

Conversation

@NimaSarajpoor
Copy link
Collaborator

@NimaSarajpoor NimaSarajpoor commented Jun 6, 2022

This PR is to investigate the time-out failure in PR #595.

This is how I created this branch:

# in branch main
git branch -c [old branch] [new branch]
git checkout [new branch]

# so, while  I am in this new branch
git reset --hard HEAD~[number]

Note that I did not git merge main after this. It seems the changes in main are preserved already. So, later, we can skip the commits regarding the git merge main.

@NimaSarajpoor
Copy link
Collaborator Author

NimaSarajpoor commented Jun 6, 2022

I got into a detached head problem where things got weird and the changes got disappeared. So, I had to retrieve the changes! This is basically just to make sure everything is normal again in the beginning.

(I am now using git show to find changes in each commit and apply those changes to the files in this current branch)

@seanlaw
Copy link
Contributor

seanlaw commented Jun 6, 2022

The tests passed. Please keep adding incrementally until tests fail

@seanlaw
Copy link
Contributor

seanlaw commented Jun 6, 2022

@NimaSarajpoor FYI, I just made some changes to test.sh (see commit 89487ae) so that:

  1. The coverage test results have been split up so that we have a little bit better visibility into how long each test is taking (similar to unit tests)
  2. I've updated ./test.sh custom to be a lot more user friendly from the command line so you shouldn't need to change the test.sh file and so that you can quickly do:
# Basic usage
./test.sh custom tests/test_stump.py

# Run multiple test files
./test.sh custom tests/test_stump.py tests/test_stumped.py

# Run 1000 rounds (default is 10 rounds)
./test.sh custom 1000 tests/test_stump.py

# Run a specific test within a test file
./test.sh custom tests/test_stump.py::test_stump_self_join

@NimaSarajpoor
Copy link
Collaborator Author

@NimaSarajpoor FYI, I just made some changes to test.sh (see commit 89487ae) so that:

  1. The coverage test results have been split up so that we have a little bit better visibility into how long each test is taking (similar to unit tests)
  2. I've updated ./test.sh custom to be a lot more user friendly from the command line so you shouldn't need to change the test.sh file and so that you can quickly do:
# Basic usage
./test.sh custom tests/test_stump.py

# Run multiple test files
./test.sh custom tests/test_stump.py tests/test_stumped.py

# Run 1000 rounds (default is 10 rounds)
./test.sh custom 1000 tests/test_stump.py

# Run a specific test within a test file
./test.sh custom tests/test_stump.py::test_stump_self_join

Awesome! (off-topic note: you may want to add it to the contribution doc so others can know about this.)


I will resume pushing.

@NimaSarajpoor
Copy link
Collaborator Author

@seanlaw
I know that I should do k in range(2, 4) as k=1 is already tested; however, I am trying to replicate the commits so we do not miss anything.

(Also, for now, I am skipping the commits in which I just fixed a typo or rename a function).

@seanlaw
Copy link
Contributor

seanlaw commented Jun 6, 2022

@seanlaw I know that I should do k in range(2, 4) as k=1 is already tested; however, I am trying to replicate the commits so we do not miss anything.

(Also, for now, I am skipping the commits in which I just fixed a typo or rename a function).

Sounds good. Thanks for the heads up

@seanlaw
Copy link
Contributor

seanlaw commented Jun 6, 2022

Great! Tests are still passing

@NimaSarajpoor
Copy link
Collaborator Author

I will push the next commit in a couple of hours.

@NimaSarajpoor
Copy link
Collaborator Author

NimaSarajpoor commented Jun 7, 2022

@seanlaw
I think we are going to see failure in this last commit as it is related to a function that is used in stumped, and I believe the remaining commits are just minor changes.
(Again, I avoided changing anything. For instance, I noticed that I should have done PB[:, :] = np.sort(PB, axis=1) in line1074, but I did not change it.)


If we do not see any failure this time, I will push the few remaining commits together.

@NimaSarajpoor
Copy link
Collaborator Author

I think I already covered all commits!! I do not know why tests are passing., this last commit was just fixing typos.

How about this?

git checkout TopK_MatrixProfile

# so, I go to branch `TopK_MatrixProfile`
git merge TopK_MatrixPrfile_Investigator

Them, I can take a look at conflicts (if there is any) and see what's going on.

@seanlaw
Copy link
Contributor

seanlaw commented Jun 7, 2022

I do not know why tests are passing.

@NimaSarajpoor I'd bet that it is because of the two things that I did:

  1. Switch to 2022.5.0 for both dask and distributed
  2. Split the coverage tests into individual tests

Let me revert the first one (i.e., allow 2022.5.2 to be installed) and then have you merge and push to see what happens.

Note that somebody submitted a fix for the dask/distributed regression in #617. Once I merge this, then I expect the longer running time to be resolved but I will hold off until we test these things first

@NimaSarajpoor
Copy link
Collaborator Author

@seanlaw
I also compared the files side by side and it seems they are the same (i.e. I did not miss any commit and all of them are replicated here in this PR).

@seanlaw
Copy link
Contributor

seanlaw commented Jun 7, 2022

I also compared the files side by side and it seems they are the same (i.e. I did not miss any commit and all of them are replicated here in this PR).

I think this is pretty good confirmation that the problem was not likely to be from your code. Let's flip back to the other repo

@NimaSarajpoor
Copy link
Collaborator Author

NimaSarajpoor commented Jun 7, 2022

I am planning to delete this branch. Just to be on the safe side, I will back up the files in stumpy and tests (of branch TopK_MatrixProfile)

If there is something that I should be aware of regarding this matter, please let me know.

@seanlaw
Copy link
Contributor

seanlaw commented Jun 8, 2022

No, I think it should be okay to delete this branch and close out this PR

@NimaSarajpoor NimaSarajpoor deleted the TopK_MatrixProfile_Investigator branch June 8, 2022 02:29
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.

3 participants