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

Output for gridded data subarea and line #566

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

Conversation

stcui007
Copy link
Contributor

@stcui007 stcui007 commented Jul 12, 2023

This PR incorporates python module on grids into ngen framework with user selected rectangular area with arbitrary size and location (2d); or a line along the longitude or latitude direction (1d), output the data in csv format. The key function is to build an index array for output utilizing BMI grid functions and uses the index array to output variables. The index array can be easily extended to higher dimentionality.

Additions

example_bmi_multi_realization_config_w_grids.json

Removals

Changes

include/bmi/Bmi_Py_Adapter.hpp
include/realizations/catchment/Bmi_Formulation.hpp
include/realizations/catchment/Bmi_Module_Formulation.hpp
include/realizations/catchment/Bmi_Multi_Formulation.hpp
src/realizations/catchment/Bmi_Py_Formulation.cpp
test/realizations/catchments/Bmi_Py_Formulation_Test.cpp

Testing

  1. Unit tests
  2. Run ngen tests

Screenshots

Notes

Todos

Checklist

  • [x ] PR has an informative and human-readable title
  • [x ] Changes are limited to a single goal (no scope creep)
  • [x ] Code can be automatically merged (no conflicts)
  • [x ] Code follows project standards (link if applicable)
  • [x ] Passes all existing automated tests
  • [x ] Any change in functionality is tested
  • [x ] New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • [x ] Linux

@PhilMiller
Copy link
Contributor

I would lean against continuing this PR as it stands. I don't think get_output_line_for_timestep should exist as a member of catchment formulations at all. There is nothing about "simulate the hydrology of a catchment" that has anything to do with text formatting.

@robertbartel
Copy link
Contributor

I would lean against continuing this PR as it stands. I don't think get_output_line_for_timestep should exist as a member of catchment formulations at all. There is nothing about "simulate the hydrology of a catchment" that has anything to do with text formatting.

I started to get on a soap box about something tangental, but then something (re)occurred to me.

@PhilMiller's comment relates to something I've brought up a few times in the past: it's something that I think might be helped if we added more robust configuration modeling components as their own separate entities.

In the realization config, it makes sense to control output contents and formatting for a catchment at the level of the formulation. But there isn't any separation between the formulation's config and the formulation domain science object itself, after the config is loaded from JSON.

A problem for this, though, is that BMI causes some things to be tightly coupled beyond our control. The validity of any particular output arrangement (and some other formulation config details) is bound to the particular BMI formulation instance and the underlying module(s). That does create a bit of a caveat to Phil's last assertion: the simulator has to be involved when determining whether the format requested for output makes any sense.

Phil's core point is still correct, and we could still benefit from separating certain concerns into distinct components. Properly addressing either/both is just going to be a little tricky.

@stcui007
Copy link
Contributor Author

stcui007 commented Jan 11, 2024 via email

@hellkite500
Copy link
Contributor

Once rebased, convert to draft and wait to see how/if this integrates with incoming changes based on, currently, #676

@stcui007 stcui007 force-pushed the gridded_output_subarea branch from 562eb43 to 1699ac7 Compare January 16, 2024 16:17
@stcui007 stcui007 force-pushed the gridded_output_subarea branch from db97c97 to f28f7f4 Compare January 17, 2024 15:13
@stcui007 stcui007 marked this pull request as draft January 22, 2024 19:40
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.

4 participants