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

Broadcast scalar input variable values to expected array structure #556

Merged
merged 5 commits into from
Jul 3, 2023

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Jun 30, 2023

Extracted from #534 to more directly clear the way for #536

Fixes #541, #549

Fixes a number of invalid memory access resulting from the receiving model trying to read more data that the framework is providing.

When built and configured with export CXXFLAGS="-g -fsanitize=address" CFLAGS="-g -fsanitize=address" FFLAGS="-g", ctest reports as follows

  • Before:
98% tests passed, 14 tests failed out of 821

[...]

The following tests FAILED:
	233 - Bmi_Fortran_Formulation_Test.GetOutputLineForTimestep_0_a (Subprocess aborted)
	291 - Bmi_Py_Formulation_Test.get_response_0_a (Subprocess aborted)
	292 - Bmi_Py_Formulation_Test.get_response_0_b (Subprocess aborted)
	294 - Bmi_Py_Formulation_Test.GetOutputLineForTimestep_0_a (Subprocess aborted)
	295 - Bmi_Py_Formulation_Test.GetOutputLineForTimestep_0_b (Subprocess aborted)
	315 - Bmi_Multi_Formulation_Test.GetOutputLineForTimestep_1_a (Subprocess aborted)
	316 - Bmi_Multi_Formulation_Test.GetOutputLineForTimestep_1_b (Subprocess aborted)
	460 - Bmi_Fortran_Formulation_Test.GetOutputLineForTimestep_0_a (Subprocess aborted)
	518 - Bmi_Py_Formulation_Test.get_response_0_a (Subprocess aborted)
	519 - Bmi_Py_Formulation_Test.get_response_0_b (Subprocess aborted)
	521 - Bmi_Py_Formulation_Test.GetOutputLineForTimestep_0_a (Subprocess aborted)
	522 - Bmi_Py_Formulation_Test.GetOutputLineForTimestep_0_b (Subprocess aborted)
	542 - Bmi_Multi_Formulation_Test.GetOutputLineForTimestep_1_a (Subprocess aborted)
	543 - Bmi_Multi_Formulation_Test.GetOutputLineForTimestep_1_b (Subprocess aborted)
  • After:
100% tests passed, 0 tests failed out of 821

Changes

  • A variable provided as a scalar value from a source will be broadcast to an array of the recipient model's expected size.

Notes

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • 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 ➡️

@PhilMiller
Copy link
Contributor Author

@hellkite500 I opened this PR at Matt's request to separate it for easier review from #534. As we discussed, there need to be better guard rails and/or reporting that this is happening, so that it doesn't silently trip anyone up.

@PhilMiller
Copy link
Contributor Author

The added warning about the broadcasting appears this many times in the tests:

$ grep broadcasting output | sort | uniq -c | sort -rn
1086 544: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
1086 543: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
1086 317: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
1086 316: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
 544 522: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
 544 295: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
 543 541: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
 543 462: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
 543 314: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
 543 235: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  78 539: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  78 538: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  78 536: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  78 312: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  78 311: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  78 309: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  39 534: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  39 519: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  39 458: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  39 307: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  39 292: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
  39 231: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 542: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 537: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 535: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 459: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 315: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 310: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 308: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   2 232: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 540: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 533: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 521: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 518: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 461: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 460: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 457: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 313: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 306: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 294: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 291: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 234: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 233: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array
   1 230: ngen Warning: broadcasting variable 'GRID_VAR_1' from scalar to expected array

@PhilMiller PhilMiller force-pushed the promote-scalar-inputs branch from 593b534 to 23174f4 Compare June 30, 2023 22:14
@PhilMiller
Copy link
Contributor Author

(rebased, and we now get 100% test pass)

@mattw-nws mattw-nws marked this pull request as ready for review July 3, 2023 18:44
@mattw-nws
Copy link
Contributor

This seems perfectly sound other than the philosophical question of whether we should be doing it... which (for the record) remains undecided. Approving this in any case though because the existing code produces undefined behavior which is decidedly worse.

@mattw-nws mattw-nws self-requested a review July 3, 2023 18:47
@mattw-nws mattw-nws merged commit f4d0ff3 into NOAA-OWP:master Jul 3, 2023
@PhilMiller PhilMiller deleted the promote-scalar-inputs branch July 5, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants