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

Refactor Model initialization #528

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SouthEndMusic
Copy link
Contributor

@SouthEndMusic SouthEndMusic commented Jan 16, 2025

Issue addressed

Fixes #523

Explanation

Explain how you addressed the bug/feature request, what choices you made and why.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with master
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.qmd if needed

Additional Notes (optional)

Some general bits of code advise (opinion):

  • give global constants names in ALL_CAPS so that they are recognizable as global constants
  • I like to make function signatures as explicit as possible, not for dispatch but just to make very clear what kind of objects the function is acting on

@SouthEndMusic SouthEndMusic marked this pull request as draft January 16, 2025 08:44
@SouthEndMusic SouthEndMusic changed the title Refactor sbm network initialization Refactor Model initialization Jan 16, 2025
Comment on lines 193 to 194
area = cell_data.x_length .* cell_data.y_length,
slope = land_slope,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest to move these parameters to LandGeometry for aligning sbm and sbm_gwf with sediment.

land_indices = inds_land_map2river,
lake_indices = lake_data.inds_lake_map2river,
allocation_area_indices = river_allocation_area_inds,
cell_area = cell_data.x_length[inds_land_map2river] .*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest to move this parameter to RiverGeometry.

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.

Refactor and align Model initialization functions
2 participants