Issue #1519 weights in label arrays #1520
Conversation
…ured grids via xugrid
…of release on conda-forge
|
| of ``Ugrid2d.from_structured`` if available, to save some costly | ||
| conversions. | ||
| """ | ||
| weights_uda = as_ugrid_dataarray(weights) |
There was a problem hiding this comment.
I'm wondering what the performance impact is of converting this. Have you tested it? Models that are being split are usually big
There was a problem hiding this comment.
Good point. I've tested this before, as we also convert structured grids to unstructured grids it for the horizontal flow barrier package (to snap to edges). There, it posed a performance hit as the conversion had to be called multiple times (the LHM has ~20 HFB packages). That's why as_ugrid_dataarray caches the result of this conversion, so it doesn't have to be converted multiple times. I think for the LHM (a very large model > 20M cells), it is bearable to call the conversion once. It takes about a minute for that.
I also call as_ugrid_dataarray here, as it might save some performance in the case users also work with a HFB package.
Fixes #1519 and #668
Description
Implements the following:
get_label_arraytoimod.prepare.create_partition_labels, and add this to public API. Calling the oldget_label_arrayresults in a DeprecationWarning, after which args are passed on toimod.prepare.create_partition_labels.I was still on the fence about the latter, on one hand, I like that these changes as they keep the code paths very similar and support providing weights, on the other hand, the way Metis is now called, it results in sloppy looking label arrays for structured grids.
For this trivial example with idomain equal to one everywhere. We had this:
and with METIS we get this:
That is still somewhat acceptable, though confusing as it looks sloppy, so it deserves some extra explanation.
Perhaps manually create a simple label array here is better?
For the hondsrug example, which is not a perfect square model, METIS accounts for the fact that the model domain is rectangular, which the previous implementation does not:
So in the end I think it is an improvement
Checklist
Issue #nr, e.g.Issue #737