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

Avoid having randomized unittest #333

Open
skim0119 opened this issue Feb 20, 2024 · 17 comments
Open

Avoid having randomized unittest #333

skim0119 opened this issue Feb 20, 2024 · 17 comments
Assignees
Labels
github_actions Pull requests that update GitHub Actions code good first issue Good for newcomers prio:low Priority level: low

Comments

@skim0119
Copy link
Collaborator

skim0119 commented Feb 20, 2024

  • This issue came up on one of the github-action.
  • Instead of random test, we should have pre-generated set of test cases and known solution.
  • Precision should be specified.
  • Best to include corner cases and negative cases (fail case)

random_start_position = np.random.randn(3, 1)
random_end_position = np.random.randn(3, 1)
random_directors, _ = np.linalg.qr(np.random.randn(3, 3))
random_directors = random_directors.reshape(3, 3, 1)
rod_like_system = make_simple_system_with_positions_directors(
random_start_position, random_end_position, random_directors
)

@skim0119 skim0119 added good first issue Good for newcomers prio:low Priority level: low github_actions Pull requests that update GitHub Actions code labels Feb 20, 2024
@vikas-chaudhary-2802
Copy link

Hi, I love to work on this issue, Can you please assign me ??

@bhosale2
Copy link
Collaborator

@vikas-chaudhary-2802 the issue has been assigned, please follow these guidelines when working on the issue.

@skim0119
Copy link
Collaborator Author

@vikas-chaudhary-2802 I included some more explanation above.

@vikas-chaudhary-2802
Copy link

@skim0119 I have made few changes and i wanted to test whether those are working. Do i need to build the project and then run the test using 'make test' command?
If yes, how can i build it?

@skim0119
Copy link
Collaborator Author

skim0119 commented Feb 28, 2024

@vikas-chaudhary-2802 Repository can be installed using pip or poetry.

@vikas-chaudhary-2802
Copy link

@skim0119 whether I need to build after I make the changes or not ?

@skim0119
Copy link
Collaborator Author

@vikas-chaudhary-2802 If you have made changes within the tests directory, it won't be necessary to reinstall.

@vikas-chaudhary-2802
Copy link

vikas-chaudhary-2802 commented Mar 5, 2024

@skim0119 i added logging line to check the datatype but the logs are not getting printed on the shell when i run - make tests....could you please suggest any other way to debug ?

@skim0119
Copy link
Collaborator Author

skim0119 commented Mar 6, 2024

@vikas-chaudhary-2802 You can directly run pytest -s on repository to capture stdout/stderr

@vikas-chaudhary-2802
Copy link

@skim0119 @bhosale2 I'm facing difficulties with certain test cases and despite my attempts, I haven't been able to resolve them. Could you please provide assistance with these specific issues?
Screenshot 2024-03-18 at 9 50 05 AM

@skim0119
Copy link
Collaborator Author

@vikas-chaudhary-2802 Could you commit your code in your fork that reproduces this error?

@vikas-chaudhary-2802
Copy link

@skim0119 yeah, I am able to commit
Screenshot 2024-03-19 at 5 33 11 PM

@vikas-chaudhary-2802
Copy link

@skim0119 @bhosale2 I Create a PR, Can you please suggest me the changes that i have to do ...so that the failed test cases will resolve and then i will commit the changes again

@skim0119
Copy link
Collaborator Author

@vikas-chaudhary-2802 I went over your code. The directors passed to the timestepper need to be orthogonal. I think that is why the analytical solution is breaking. Please correct the directors and try to run the test again.

Also, try to format the script before commiting. You can run make format-codestyle.

@vikas-chaudhary-2802
Copy link

Hi, @skim0119 Thank You for your suggestion. I will make the changes as soon as possible

@vikas-chaudhary-2802
Copy link

vikas-chaudhary-2802 commented Mar 26, 2024

Hi, @skim0119 @bhosale2 Now the directors passed to the timestepper is orthogonal but still the same test cases failed.... i will attached the changes ....please review it and tell me what i have to do next to pass the test cases as soon as possible

Screenshot 2024-03-27 at 12 45 47 AM

@skim0119
Copy link
Collaborator Author

skim0119 commented Mar 26, 2024

@vikas-chaudhary-2802 I would appreciate it if you could attach the actual code, or link the forked script to re-run. I can't help out just looking at the image file. Same for the error message.

Few comments:

  • I am not sure how you are initializing random_directors. Please re-write them readable and clean. Quickly looking at it, it looks wrong implementation. The final dimension needs to be 3x3xN where N is the number of test cases.
  • Don't use QR decomposition to orthogonalize them. The whole point of this issue and avoiding randomized unit-test is to test if the code can handle largely expected cases (and corner cases if any). Your approach, in the end, has the same issue as the original implementation: don't know why the test passes or fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code good first issue Good for newcomers prio:low Priority level: low
Projects
None yet
Development

No branches or pull requests

3 participants