NREL-Sienna / InfrastructureSystems.jl

Utility package for Sienna's simulation infrastructure
https://nrel-sienna.github.io/InfrastructureSystems.jl/
BSD 3-Clause "New" or "Revised" License
38 stars 20 forks source link

Make time series simulation field optional #269

Closed josephmckinsey closed 2 years ago

josephmckinsey commented 2 years ago

This simulation field seems to have no purpose. We could also remove it very easily (see my branch remove-simulation-field).

codecov[bot] commented 2 years ago

Codecov Report

Merging #269 (3224131) into master (3788c89) will decrease coverage by 0.27%. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
- Coverage   78.54%   78.27%   -0.28%     
==========================================
  Files          43       43              
  Lines        3570     3438     -132     
==========================================
- Hits         2804     2691     -113     
+ Misses        766      747      -19     
Flag Coverage Δ
unittests 78.27% <50.00%> (-0.28%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/time_series_parser.jl 70.00% <50.00%> (-0.74%) :arrow_down:
src/InfrastructureSystems.jl 77.77% <0.00%> (-5.56%) :arrow_down:
src/utils/utils.jl 64.58% <0.00%> (-1.57%) :arrow_down:
src/utils/print.jl 52.10% <0.00%> (-1.10%) :arrow_down:
src/utils/generate_structs.jl 84.68% <0.00%> (-0.91%) :arrow_down:
src/internal.jl 78.84% <0.00%> (-0.79%) :arrow_down:
src/probabilistic.jl 77.19% <0.00%> (-0.78%) :arrow_down:
src/scenarios.jl 83.67% <0.00%> (-0.65%) :arrow_down:
src/deterministic.jl 81.53% <0.00%> (-0.56%) :arrow_down:
src/utils/recorder_events.jl 91.39% <0.00%> (-0.53%) :arrow_down:
... and 18 more
daniel-thom commented 2 years ago

Would you mind also setting a default value for the parameter in the TimeSeriesFileMetadata kwarg constructor? We have some exported function that accept this type, and so this change would ensure we never have a problem with the parameter.

daniel-thom commented 2 years ago

@claytonpbarrows Any updated opinion since yesterday?

josephmckinsey commented 2 years ago

Would you mind also setting a default value for the parameter in the TimeSeriesFileMetadata kwarg constructor? We have some exported function that accept this type, and so this change would ensure we never have a problem with the parameter.

Hopefully we don't have a similar problem with TimeSeriesParsedInfo, since simulation is not a kwarg there so making a default parameter would be a breaking change for any callers.

daniel-thom commented 2 years ago

Would you mind also setting a default value for the parameter in the TimeSeriesFileMetadata kwarg constructor? We have some exported function that accept this type, and so this change would ensure we never have a problem with the parameter.

Hopefully we don't have a similar problem with TimeSeriesParsedInfo, since simulation is not a kwarg there so making a default parameter would be a breaking change for any callers.

I think we're OK there because it's only used internally - and only in InfrastructureSystems.

claytonpbarrows commented 2 years ago

LGTM, thanks everyone