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

Incorrect value used to fill missing layers in angle1 array in NPF package #1395

Closed
JoerivanEngelen opened this issue Jan 29, 2025 · 3 comments · Fixed by #1396
Closed

Incorrect value used to fill missing layers in angle1 array in NPF package #1395

JoerivanEngelen opened this issue Jan 29, 2025 · 3 comments · Fixed by #1396
Assignees
Labels
bug Something isn't working
Milestone

Comments

@JoerivanEngelen
Copy link
Contributor

Bug description
Users do not have to set values for each layer for the ANI package in the projectfile. We have utilities to fill these missing layers with a constant value. Right now we use the incorrect value to fill missing layers: Should be 90, is 0 at the moment.

@JoerivanEngelen JoerivanEngelen added the bug Something isn't working label Jan 29, 2025
@github-project-automation github-project-automation bot moved this to 📯 New in iMOD Suite Jan 29, 2025
@JoerivanEngelen JoerivanEngelen self-assigned this Jan 29, 2025
@JoerivanEngelen JoerivanEngelen moved this from 📯 New to 🏗 In Progress in iMOD Suite Jan 29, 2025
@Huite
Copy link
Contributor

Huite commented Jan 29, 2025

No, this is not the right approach!
By filling a default value of 90, MODFLOW 6 has do a great deal of unnecessary work. Unless I'm mistaken, this potentially results in expensive trigoniometric evaluations, potentially for each formulation!
You should do the trig in imod-python instead, and rotate the north bound iMOD5 kh to the eastbound MF6 k11.

@JoerivanEngelen
Copy link
Contributor Author

JoerivanEngelen commented Jan 31, 2025

Thanks @Huite for ringing the alarm on this one. The approach taken from iMOD5 in imod.mf6.NodePropertyFlow.from_imod5_data is suboptimal.

In my short description I did not make my priorities and scope of this issue clear:

This issue is a subissue of parent #1208. My idea for this one is to first reproduce iMOD5 input as much as possible (except when it would lead to lots of new code implementing perfidious things). I use this to see convert the LHM and run it to see if there are any noticeable differences and detect potential bugs in iMOD Python. In the current state of the code base, the implementation is already suboptimal and has the wrong default value for missing layers. Fixing this default value is as I proposed is trivial and at least makes things consistent with iMOD5, so we can avoid any doubt that differences might be caused by incorrect trigoniometric computations in iMOD Python. This is my top priority now.

In a follow-up issue, we can replace the current suboptimal implementation with a better one. This will be picked up when I've resolved #1208 and proven that iMOD Python does the right thing for the LHM in its conversion of iMOD5 data to MODFLOW6 data. (I think I'm nearly there). I'll open up a follow-up issue to optimize the NPF import.

@JoerivanEngelen JoerivanEngelen added this to the v1.0 release milestone Jan 31, 2025
@JoerivanEngelen
Copy link
Contributor Author

I created #1397 to resolve the issues @Huite raised.

github-merge-queue bot pushed a commit that referenced this issue Jan 31, 2025
Fixes #1395

# Description
Use proper value for missing layers in projectfile.

# Checklist
<!---
Before requesting review, please go through this checklist:
-->

- [x] Links to correct issue
- [x] Update changelog, if changes affect users
- [x] PR title starts with ``Issue #nr``, e.g. ``Issue #737``
- [x] Unit tests were added
- [ ] **If feature added**: Added/extended example
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in iMOD Suite Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants