-
Notifications
You must be signed in to change notification settings - Fork 0
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
Golnesa variable friction #346
Conversation
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 went trough the code line by line and left some comments. It looks good, some final touches for finishing it. Notably:
- Use pre-commit for static code analysis (linting)
- Preserve the call signatures of the tabulator functions (seperate out the insertion of the veg coefficients)
Please make sure the code works, confirm it by manually running it and optionally integrate it in the automated integration tests (you could for instance adjust the sqlite that is checked in into the repo)
@@ -43,36 +49,47 @@ def convert(self, ids): | |||
content_pk=ids, | |||
code=self.code[idx], | |||
count=0, | |||
count_yz = 0, |
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 run the black
code formatter? There's a pre-commit
configuration in this repo, so after you installed the pre-commit tool (https://pre-commit.com/), you have to do pre-commit install
inside the repository and then pre-commit run all
|
||
|
||
def tabulate_yz(shape, width, height): | ||
def tabulate_yz(shape, width, height, friction_values, vegetation_stem_densities, vegetation_stem_diameters, vegetation_heights, vegetation_drag_coefficients): |
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.
This call signature doesn't match the other call signatures (at line 68 you did adjust the altered return value, but the new parameters aren't provided)
I think it is good to leave the call signature as is and provide the additional vegentation coefs at a different moment.
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.
Lots of new stuff! :) Almost there, some small remarks and questions but overall it looks good!
No description provided.