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(dis*): flatten idomain and move to base type #2158

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wpbonelli
Copy link
Contributor

Store idomain as a 1D array in DisBaseType, rather than the proper grid shape in the concrete types. This deduplicates the variable and simplifes grid equivalence checks added in #2149 and planned in #2153.


Checklist of items for pull request

  • Replaced section above with description of pull request
  • Formatted new and modified Fortran source files with fprettify
  • Added doxygen comments to new and modified procedures
  • Removed checklist items not relevant to this pull request

Store idomain as a 1D array in DisBaseType, rather than the proper grid shape in the concrete types. This deduplicates the variable and simplifes grid equivalence checks.
@wpbonelli wpbonelli changed the title refactor(dis): flatten idomain and move to base type refactor(dis*): flatten idomain and move to base type Jan 22, 2025
Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

Sadly, this seems to have required more work than I would have thought. I guess because of the mem_set_value, you need to allocate a local idomain multidimensional array and then copy those values into the 1d disbase member. And the changes also propagate into the converter.

@wpbonelli wpbonelli added the code refactor Nonfunctional changes label Jan 23, 2025
@mjr-deltares
Copy link
Contributor

A quick note: this will have consequences for anyone using IDOMAIN in its current shapes through the API. Personally, I have code that relies on the current (multi-dim) arrays, for example, to render VTK files. This will break then. I am not trying to block this, because I can easily adapt, just wanted to share the heads up.

@wpbonelli
Copy link
Contributor Author

After chatting with @mjr-deltares we thought this should wait, at least until a non-bugfix release, since it will break API usages. Maybe also consider alternatives.

@langevin-usgs

@wpbonelli wpbonelli added onhold Waiting for something question Needs someone's attention labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactor Nonfunctional changes onhold Waiting for something question Needs someone's attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants