Skip to content

Conversation

@augustposch
Copy link
Collaborator

Added some comments throughout.
In generate_flood_from_discharge() within hydrodynamic_prediction.py, I believe we were missing a division by 3600 for units conversion. But please check over that spot and make sure what I commented was logical. If I missed something send it back, I'll be looking at it again Thursday afternoon

@augustposch augustposch requested a review from annehaley October 2, 2025 13:10
@augustposch
Copy link
Collaborator Author

Hi Anne, hold on, I found another units issue around q - output of calculate_discharge_from_precipitation() is in cubic feet per second, but the input of generate_flood_from_discharge() requires q in cubic feet per day. I'll fix it and make a new pull request.

@augustposch
Copy link
Collaborator Author

Hi Anne, I just committed again and I think I've solved the last problem with units. However, I was not able to verify that the whole thing works as intended because of issues with my environment. Perhaps you could run this branch from your command line and see how it works for you? Btw, if the environment is figured out, then the units change means the flood magnitude should be 24 times what you had before, which might answer your question about the flood looking unexciting.

Copy link
Contributor

@annehaley annehaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment on the README, but otherwise LGTM. Thank you for adding all of this explanation. I made sure to update this branch with main so that it has one minor change to the arguments.

I did try this out and you were right, the resulting flood has a lot more variation. Thank you for fixing the units.

@augustposch augustposch merged commit 093e31b into main Oct 3, 2025
@augustposch augustposch deleted the comment-explanations-branch branch October 3, 2025 16: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