Skip to content

add ground temperature 1m as option to weather data #1351

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open

add ground temperature 1m as option to weather data #1351

wants to merge 32 commits into from

Conversation

@pierrepetersmeier
Copy link
Contributor

Resolves #1343

@pierrepetersmeier pierrepetersmeier self-assigned this Jun 26, 2025
@danielfeismann danielfeismann added enhancement New feature or request labels Jun 26, 2025
danielfeismann
danielfeismann requested changes Jul 3, 2025
View reviewed changes
Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

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

Didn't get to all of it. Just a first view thoughts that imho needs to be adapted. As well it might get easier since it would reduce the amount of changes.

@danielfeismann danielfeismann added this to the Version 8.2 milestone Jul 25, 2025
danielfeismann
danielfeismann requested changes Aug 14, 2025
View reviewed changes
Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

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

This looks already quite ok, but I guess some parts can be improved.

@pierrepetersmeier pierrepetersmeier marked this pull request as ready for review October 19, 2025 20:06
danielfeismann
danielfeismann requested changes Oct 20, 2025
View reviewed changes
Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

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

Some comments and questions from my side. But it is getting into the right direction!

Could you please provide some information why choosing 0 cm and 80 cm and some more information on this at all to the documentation and adapt existing weather information in the docs (see weathersource.md)? Maybe it's worth to start a weatherdata document?

src/main/java/edu/ie3/datamodel/models/value/WeatherValue.java Outdated
Comment on lines 143 to 150
public Optional<GroundTemperatureValue> getGroundTemperature0cm() {
return Optional.ofNullable(groundTemperature0cm);
}

public Optional<GroundTemperatureValue> getGroundTemperature80cm() {
return Optional.ofNullable(groundTemperature80cm);
}

Copy link
Member

Choose a reason for hiding this comment

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

these methods are unsed, but if you look to e.g. getTemperature they are used in the tests. Please add these also to tests (and if you think there can be additional tests, please add them as well)

...3/datamodel/io/source/csv/_weather/icon/its_weather_513606bc-539e-445b-9675-2f98be3d9231.csv Outdated
2019-08-01T16:00:00Z,13.015240669,348.84439309613776,200.46049098038043,56.00436431107297,204.38963365625,297.3200023473353,298.8447737622156,2.557259341952788,4.0165196739267675,2.5543417132442308,4.204610497390883,3.6709121158156393,-0.38463576259530396,-0.5748064219197632,-0.4001297004267148,-0.574231301551345,-0.5484601012731134,0.008420781588303635,0.004028919955548831,0.0103738560877878,0.0064212084500956355,0.9553236526118866,67775,,,,,
2019-08-01T16:00:00Z,13.013334274000002,287.1635211775765,241.641483540946,46.18262289142059,91.70939132296976,293.455111314491,294.9876832274387,2.2429257128168105,3.288655459909278,2.2469304566362895,3.456196177795884,3.063505298416552,0.7052856075394569,1.0173658432825086,0.6940309956521304,1.0831645239762804,0.9402700184046242,-0.009609051331189665,-0.0037207462780739073,-0.01264290152206423,-0.006439463432156433,0.9553365723370035,67776,,,,,
2019-08-01T17:00:00Z,13.015240669,306.57139450950467,180.73429610400223,49.19860365549343,175.039569078125,296.8287403584074,297.6596017457568,2.2517126616190337,3.650669504895637,2.2438962003705027,3.7980736030429303,3.339152911211416,-0.6950893529619305,-1.1085323450143223,-0.7122478653505989,-1.1293057574368208,-1.0352309009257914,0.012464921655326946,0.0059655751175761145,0.015265360298047703,0.009632113129412919,0.9553227603369525,67775,,,,,
"time","alb_rad","asob_s","aswdifd_s","aswdifu_s","aswdir_s","t_2m","t_g","t80cm","u_10m","u_131m","u_20m","u_216m","u_65m","v_10m","v_131m","v_20m","v_216m","v_65m","w_131m","w_20m","w_216m","w_65m","z0","coordinate_id","p_131m","p_20m","p_65m","sobs_rad","t_131m"
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should stick to one convention, either tG or t_0cm. Here and also within the code. And I guess t_80cm (with underline) should also work, right?

src/test/groovy/edu/ie3/test/common/TimeSeriesTestData.groovy
UUID.fromString("a4bbcb77-b9d0-4b88-92be-b9a14a3e332b"),
ColumnScheme.ENERGY_PRICE
)

Copy link
Member

Choose a reason for hiding this comment

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

Please also add some test cases with ground temperatures (keep in might to test also for combinations that now may arise)

...3/datamodel/io/source/csv/_weather/icon/its_weather_513606bc-539e-445b-9675-2f98be3d9231.csv Outdated
2019-08-01T17:00:00Z,13.015240669,306.57139450950467,180.73429610400223,49.19860365549343,175.039569078125,296.8287403584074,297.6596017457568,2.2517126616190337,3.650669504895637,2.2438962003705027,3.7980736030429303,3.339152911211416,-0.6950893529619305,-1.1085323450143223,-0.7122478653505989,-1.1293057574368208,-1.0352309009257914,0.012464921655326946,0.0059655751175761145,0.015265360298047703,0.009632113129412919,0.9553227603369525,67775,,,,,
"time","alb_rad","asob_s","aswdifd_s","aswdifu_s","aswdir_s","t_2m","t_g","t80cm","u_10m","u_131m","u_20m","u_216m","u_65m","v_10m","v_131m","v_20m","v_216m","v_65m","w_131m","w_20m","w_216m","w_65m","z0","coordinate_id","p_131m","p_20m","p_65m","sobs_rad","t_131m"
"2019-08-01T15:00:00Z",13.015240669,503.46974264373205,228.021339757131,80.8246124780934,356.2648859375,297.6241992659816,300.6632065668998,289.5,2.594603775363224,3.7658971156831287,2.5812495613105044,3.941521213236469,3.4740205817325034,-0.024078646721241395,-0.029760831916596106,-0.052967885304510534,-0.009698125518755707,-0.04996610799324721,0.004091443774095653,0.0015809058504647026,0.005954484657501378,0.002666343696204668,0.9553221665631989,67775,,,,,
"2019-08-01T15:00:00Z",13.013334274000002,498.219742300774,245.24079037841295,80.0782271098217,333.0547140625,295.515335568404,297.43684351873816,289.2,2.6907481330116054,4.001601219938975,2.6888332994845783,4.140469437423411,3.714032263672762,1.2097386659833593,1.8148233176685413,1.1963736417917374,1.8944592514336933,1.667063608988315,-0.010735134459808796,-0.006350491263316599,-0.012234044440804974,-0.009044908476315713,0.955336762972383,67776,,,,,
Copy link
Member

Choose a reason for hiding this comment

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

We also should test for these values. (CsvWeatherSourceIconTest + ...CosmoTest...

pleae also have a look into InconWeatherTestData and CosmoWeatherTestData. Plus there might be more in test/common/... about weather where it would be great to increase the testing on this.

pierrepetersmeier and others added 6 commits October 22, 2025 21:28
@pierrepetersmeier @danielfeismann
Apply suggestion from @danielfeismann
71ab33a 
Co-authored-by: Daniel Feismann <98817556+danielfeismann@users.noreply.github.com>
@pierrepetersmeier @danielfeismann
Update src/test/resources/edu/ie3/datamodel/io/source/csv/_weather/co…
224aa30 
…smo/its_weather_8bc9120d-fb9b-4484-b4e3-0cdadf0feea9.csv

Co-authored-by: Daniel Feismann <98817556+danielfeismann@users.noreply.github.com>
@pierrepetersmeier @danielfeismann
Update src/test/resources/edu/ie3/datamodel/io/sink/_sql/time_series.sql
6039cc2 
Co-authored-by: Daniel Feismann <98817556+danielfeismann@users.noreply.github.com>
@pierrepetersmeier
remove unused function
46fb11f
@pierrepetersmeier
Merge branch 'dev' into pp/#1343-add-ground-temperature-1m-as-option-…
082451a 
…to-weather-data
@pierrepetersmeier
remove unused function and refactoring tests
3a0e1e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielfeismann danielfeismann danielfeismann requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@pierrepetersmeier pierrepetersmeier

Labels

enhancement New feature or request

Projects

None yet

Milestone

Version 8.2

Development

Successfully merging this pull request may close these issues.

Add ground temperature (1m) as option to weather data

3 participants

@pierrepetersmeier @danielfeismann