Skip to content

Issue #974 distribute drain conductance#979

Merged
JoerivanEngelen merged 6 commits into
masterfrom
issue_#974_distribute_drain_conductance
Apr 22, 2024
Merged

Issue #974 distribute drain conductance#979
JoerivanEngelen merged 6 commits into
masterfrom
issue_#974_distribute_drain_conductance

Conversation

@JoerivanEngelen

Copy link
Copy Markdown
Contributor

Fixes #974

Description

This PR adds the options to distribute drain conductances.

  • Adds by_crosscut_thickness, by_crosscut_transmissivity, by_corrected_transmissivity options to Drain package
  • Make boundary conditions top and bottom arguments optional methods
  • Change order of arguments conductance distribution functions

Checklist

  • Links to correct issue
  • Update changelog, if changes affect users
  • PR title starts with Issue #nr, e.g. Issue #737
  • Unit tests were added
  • If feature added: Added/extended example

@luitjansl luitjansl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 comment ( but approved in advance)

option, allocated, conductance, top, bottom, k
option, allocated, conductance, top, bottom, k, elevation
)
actual = take_first_cell_in_xy_plane(actual_da)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you only compare "actual" and "expected" in the first cell. So it would make the tests more strict if arrays like "bottom" and "elevation" have a different value in the first cell than in the rest of the array. Otherwise the tests would still pass even if something went wrong in the indexing.

@JoerivanEngelen JoerivanEngelen Apr 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

By doing this, we would mainly test if the testing utility function take_first_cell_in_xy_plane does what it's supposed to do. I think this adds quite some complexity to the test cases for relatively little gains. I think it is better to allocate our constrained resources to fixing other issues.

@JoerivanEngelen JoerivanEngelen added this pull request to the merge queue Apr 22, 2024
Merged via the queue into master with commit 2cab813 Apr 22, 2024
@JoerivanEngelen JoerivanEngelen deleted the issue_#974_distribute_drain_conductance branch April 22, 2024 16:35
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.

Extend distribution of conductances options for drains

2 participants