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

Test Korakianitis mixed model #17

Merged
merged 11 commits into from
Feb 11, 2025
Merged

Test Korakianitis mixed model #17

merged 11 commits into from
Feb 11, 2025

Conversation

crangelsmith
Copy link
Collaborator

@crangelsmith crangelsmith commented Jan 28, 2025

Adding test for Korakianitis model, adding instantiation and numerical checks.

Things to review:

  • Is this a good representation of the model? It could be configured better to represent typical use.
  • Is the numerical checks enough? See line 64.

@crangelsmith crangelsmith marked this pull request as ready for review January 28, 2025 15:43
@crangelsmith crangelsmith mentioned this pull request Jan 30, 2025
1 task
@LevanBokeria
Copy link
Collaborator

Hi @MaxBalmus

  • I have updated the KorakianitisMixedModel test to now load a json file containing expected values instead of defining them inside the test code.
  • For clarity, I have removed the Naghavi model tests from this branch, and moved those to a separate branch Test Naghavi model #24.
  • I think the KorakianitisMixedModel test can be improved by (i) adding comments and explanations, (ii) improving the tests to be more comprehensive. I think for now, its worth having this test run as is so that we have at least something running while we improve the codebase in other directions (such as pre-commit hooks, etc). But we should go back to KorakianitisMixedModel tests at some point.
  • If you have a particular pointer to a tutorial file that would help me run KorakianitisMixedModel and understand it more, that would help me develop better tests later.

I'll now mark this review as done, and if you concur feel free to merge this with dev.

@MaxBalmus
Copy link
Collaborator

I had a look and everything looks good to me.

@MaxBalmus MaxBalmus merged commit a3039b6 into dev Feb 11, 2025
2 checks passed
@MaxBalmus MaxBalmus deleted the 8-KorakianitisMixedModel branch February 11, 2025 10:09
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