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

Create a "Progress" netCDF #388

Merged
merged 28 commits into from
Jan 10, 2024
Merged

Conversation

N1ckP3rsl3y
Copy link
Contributor

- New/modified function subs
	* Correct the arguments of `SW_NC_create_template()`
	* `SW_NC_create_progress()` - Create a new progress netCDF file
	* `SW_NC_set_progress()` - Mark a certain site/grid cell as completed
	* `SW_NC_check_progress()` - See if the specific site/grid cell is marked to run
- Now allow a new input netCDF file - progress.nc
- Add example for "progress.nc" in files_nc.in
- Update `SW_NC_open_files()` to open the progress netCDF with writing ability
- `SW_NC_create_template()` gains arguments for frequency and input attribute for new netCDFs
- `SW_NC_set_progress()`/`SW_DOM_SetProgress()` swaps progress variable name for variable ID
- `SW_NC_check_progress()`/`SW_DOM_CheckProgress()` swaps progress variable name for variable ID
- This new variable will store the main variable IDs within a given netCDF so retrieving that information does not happen many times a run
- Simplified `SW_NC_open_files()` and function now gets the variable ID within the netCDF which was opened
- Using the `domain.nc` file, the function will
* Make a copy and fill the new template with a given variable, dimension(s) if needed and update the global attributes

- New function `update_netCDF_global_atts()` which
* Is used by `SW_NC_create_template()`
* Updates the global attributes frequency, creation date, product, feature type, and source within the generated template
- `SW_NC_CreateProgress()` now creates a special netCDF file - progress - that keeps track of the suids that will not be/are to be/have run
- The progress variable is filled with values that match the domain variable values within `domain.nc` so
	* If a suid is to be run, the progress variable value will be 0
	* If a suid has been run, the progress variable will be 1
	* If a suid is not to be run, the progress variable will be byte _FillValue (-127)

- New function `fill_prog_netCDF_vals()` which fills the progress variable with a value pattern matching `domain.nc` (run or do not run)
- New function `write_byte_att()` which writes an attribute of type byte to a netCDF
- New function `fill_netCDF_var_byte()` - writing all values to a netCDF variable
- New function `get_single_uint_val()` - get a value of type unsigned integer (currently from a domain variable)

- Updated `SW_DOM_CreateProgress()` to call it's netCDF counterpart
- Filled both functions and are no longer a stub and with their SW_Domain.c counterparts

SW_NC_check_progress()/SW_DOM_CheckProgress()
- SW_NC_check_progress()
	* Checks a specific SUID within the progress variable to see if it should be run/simulated

- SW_DOM_CheckProgress()
	* Calls the netCDF counterpart
	* Update calls to the function, mainly due to the addition of LOG_INFO
	* Silences compiler when SW_NETCDF is not defined

SW_NC_set_progress()/SW_DOM_SetProgress
- SW_NC_set_progress()
	* When a SUID is done simulating, this function will mark the location within the progress variable as completed

- SW_DOM_SetProgress()
	* Now calls its netCDF counter part
	* Silences compiler when SW_NETCDF is not defined

- New function `get_single_byte_val()` which obtains the status at the current SUID to test

- Modified the code within `SW_DOM_SimSet()` and `SW_CTL_RunSimSet()` in response to the addition of LOG_INFO in `SW_DOM_CheckProgress()`
@N1ckP3rsl3y N1ckP3rsl3y requested a review from dschlaep January 5, 2024 21:47
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (8e3adf3) 70.68% compared to head (84dbffc) 70.51%.

Files Patch % Lines
src/SW_Control.c 9.09% 20 Missing ⚠️
src/Times.c 0.00% 4 Missing ⚠️
src/filefuncs.c 0.00% 3 Missing ⚠️
src/SW_Main_lib.c 66.66% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           release/devel_v8.0.0     #388      +/-   ##
========================================================
- Coverage                 70.68%   70.51%   -0.17%     
========================================================
  Files                        21       21              
  Lines                      5631     5650      +19     
========================================================
+ Hits                       3980     3984       +4     
- Misses                     1651     1666      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- marking status of simulation runs as succeeded/failed
- this signals that failed runs should not be simulated again, e.g., after a restart (only after the underlying problem has been resolved and those suids reset)
- define progress status
- flag_values: list of possible flag values
- flag_meanings: a string whose value is a blank separated list of descriptive words or phrases, one for each flag value
Copy link
Member

@dschlaep dschlaep left a comment

Choose a reason for hiding this comment

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

Works great, thanks! Just one question

src/SW_netCDF.c Show resolved Hide resolved
dschlaep and others added 9 commits January 7, 2024 12:02
- I introduced the mistake in commit 217226f
- C++ code error: cannot convert ‘bool’ to ‘Bool’, i.e., `!local_LogInfo.stopRun` is a "bool" and not a "Bool" as SW_DOM_SetProgress() was expecting
--> invert logic, i.e., pass error status directly as 'Bool'
- recent code development added "progress.nc" and thus incremented SW_NVARNC: this caused a buffer overflow in SW_NC_read_inputs(): fileNum was in 0 1 but numVals[] remained an array of length 1
- Rename `SW_NC_open_files()` to `SW_NC_open_dom_prog_files()` to show it only opens the domain and progress files
- `SW_NC_open_dom_prog_files()`
	* Opens the domain and progress files in write mode
	* Handles the case where the progress variable may be in its own file or in the domain file and the domain file does or does not have the variable

- `SW_NC_create_progress()`
	* Detects if a new progress template needs to be created (creates a template with `SW_NC_create_template()` and fills byte attributes)
	* Detects if a progress variable needs to be created (creates a new variable with `create_full_var()` and fills byte attributes)
	* Detects if a progress variable already exists somewhere (does nothing)
	* Does not write out progress variable attributes if the variable existed before the function call

- New local function `create_full_var()`
	* Moved the variable creation within `SW_NC_create_template()` to this function
	* Calculates dimensions to write to variable and their IDs
	* Writes given attributes to the variable
	* Can be used to add variables into a template or add a variable when a file is already well-defined (e.g., adding a progress variable to domain)

- `SW_NC_create_template()`
	* Moved dimension calculations/attribute writing/variable creation to `create_full_var()`
@dschlaep
Copy link
Member

dschlaep commented Jan 9, 2024

@N1ckP3rsl3y Do you have ideas how best to deal with our C++ test program not being very happy about some of the new functionality? e.g.,

CPPFLAGS=-DSWNETCDF make clean test_run

tells me

src/SW_netCDF.c:455:38: warning: ISO C++11 does not allow conversion from string literal to 'char ' [-Wwritable-strings]
static char
wgs84_synonyms[] = {"WGS84", "WGS 84", "EPSG:4326",
^
...
src/SW_netCDF.c:661:25: warning: temporary whose address is used as value of local variable 'start' will be destroyed at the end of the full-expression [-Wdangling]
(size_t[1]){0} : (size_t[2]){0, 0};
^~~~~~~~~~~~~~
...
src/SW_netCDF.c:1544:27: error: non-constant-expression cannot be narrowed from type 'unsigned long' to 'int' in initializer list [-Wc++11-narrowing]
int timeVertVals[] = {timeSize, vertSize}, numTimeVertVals = 2;
^~~~~~~~
src/SW_netCDF.c:1544:27: note: insert an explicit cast to silence this issue
int timeVertVals[] = {timeSize, vertSize}, numTimeVertVals = 2;
^~~~~~~~
static_cast( )
src/SW_netCDF.c:1608:11: error: cannot initialize a variable of type 'char *' with an rvalue of type 'const char '
char
impliedDomType = (dimExists("site", ncFileID)) ? "s" : "xy";
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/SW_netCDF.c:1654:24: warning: ISO C++11 does not allow conversion from string literal to 'char ' [-Wwritable-strings]
char
attFailMsg = "The attribute '%s' of the variable '%s' "
^
src/SW_netCDF.c:1674:31: warning: ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]
get_dim_val(ncFileID, "site", &SDimVal, LogInfo);
^
src/SW_netCDF.c:1679:31: error: assigning to 'Bool' from incompatible type 'bool'
dimMismatch = SDimVal != SW_Domain->nDimS;
~~~~~~~~^~~~~~~~~~~~~~~~~~~
...
src/SW_netCDF.c:1705:53: error: assigning to 'Bool' from incompatible type 'bool'
dimMismatch = latDimVal != SW_Domain->nDimY ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
src/SW_netCDF.c:1979:10: error: cannot initialize a variable of type 'Bool' with an rvalue of type 'bool'
Bool domTypeIsS = strcmp(SW_Domain->DomainType, "s") == 0;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
src/SW_netCDF.c:2088:51: warning: temporary whose address is used as value of local variable 'count' will be destroyed at the end of the full-expression [-Wdangling]
size_t *count = (strcmp(domType, "s") == 0) ? (size_t[1]){1} : (size_t[2]){1, 1};
^~~~~~~~~~~~~~
...
src/SW_netCDF.c:2129:57: warning: ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]
Bool domTypeS = Str_CompareI(SW_Domain->DomainType, "s") == 0;
^
src/SW_netCDF.c:2129:10: error: cannot initialize a variable of type 'Bool' with an rvalue of type 'bool'
Bool domTypeS = Str_CompareI(SW_Domain->DomainType, "s") == 0;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18 warnings and 8 errors generated.

dschlaep and others added 4 commits January 9, 2024 13:28
- timeStringISO8601(): string with current date and time in UTC formatted according to ISO 8601
- These fixes include
	* Casting variables to Bool
	* Casting strings to/from a constant character
	* Not using the ternary operator trick of (cast){...}
- Tests now pass when SWNETCDF is defined
- New function `SW_NC_deepCopy()`
	* Deep copying all of the netCDF information from one SW_NETCDF instance to another
	* Called in `SW_DOM_deepCopy()` when tests are being run

- Update the calls to `SW_F_init_ptrs()`/`SW_F_deconstruct()` to `SW_DOM_init_ptrs()`/`SW_DOM_deconstruct()`
- new `sw_message()`

- struct LOG_INFO gains new attribute "printProgressMsg"
* progress messages are turned off by default via `sw_init_logs()`: this is the case for gtests, STEPWAT2, and rSOILWAT2
* SOILWAT2 main() turns on the printing of these progress messages unless user requests "quiet" mode
- now reports 0 wall time -- only negative wall time values indicate failure
src/SW_netCDF.c Outdated Show resolved Hide resolved
- the message should be displayed independent of where the progress tracker variable is located (domain.nc or progress.nc)
@dschlaep dschlaep merged commit 07ba534 into release/devel_v8.0.0 Jan 10, 2024
6 of 8 checks passed
@dschlaep dschlaep deleted the feature_nc_progress branch January 10, 2024 12:53
@dschlaep dschlaep linked an issue Jun 6, 2024 that may be closed by this pull request
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.

Track progress when simulating a domain
2 participants