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.

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

Change scheme of COSMO weather data #321

Closed
wants to merge 27 commits into from
Closed

Change scheme of COSMO weather data #321

wants to merge 27 commits into from

Conversation

ckittl
Copy link
Member

@ckittl ckittl commented Apr 6, 2021 •
edited

Resolves parts of #267.

Close #319 before!

Includes:

  • Adaption of column scheme for COSMO model
  • Renaming of all classes which had Psdm in it to Cosmo
  • Consolidate weather test files
  • Offer possibility to have different naming strategies

@ckittl ckittl added bug Something isn't working invalid This doesn't seem right labels Apr 6, 2021
@ckittl ckittl added this to the Version 2.0 milestone Apr 6, 2021
@ckittl ckittl requested a review from a team April 6, 2021 16:24
@ckittl ckittl self-assigned this Apr 6, 2021
@ckittl ckittl marked this pull request as draft April 6, 2021 16:24
@ckittl ckittl added this to High priority in BugTracker Apr 6, 2021
@ckittl ckittl force-pushed the ck/#267-adaptColumnSchemeOfCOSMOWeatherData branch from bda154d to 41ce1b6 Compare April 9, 2021 06:19
@codecov
Copy link

codecov bot commented Apr 9, 2021 •
edited

Codecov Report

Merging #321 (98ae220) into dev (b0cfa27) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #321      +/-   ##
============================================
+ Coverage     77.63%   77.67%   +0.03%     
- Complexity     2137     2140       +3     
============================================
  Files           271      271              
  Lines          8505     8465      -40     
  Branches        806      805       -1     
============================================
- Hits           6603     6575      -28     
+ Misses         1496     1492       -4     
+ Partials        406      398       -8
Impacted Files Coverage Δ
.../ie3/datamodel/io/source/sql/SqlWeatherSource.java 81.48% <0.00%> (-9.55%) ⬇️
...edu/ie3/datamodel/io/source/sql/SqlDataSource.java 65.71% <0.00%> (-5.72%) ⬇️
...edu/ie3/datamodel/models/input/AssetTypeInput.java 70.00% <0.00%> (-2.73%) ⬇️
...model/models/input/container/SubGridContainer.java 69.23% <0.00%> (-2.20%) ⬇️
...atamodel/models/input/container/GridContainer.java 52.17% <0.00%> (-2.00%) ⬇️
.../ie3/datamodel/io/connectors/CsvFileConnector.java 69.69% <0.00%> (-1.82%) ⬇️
...tamodel/models/input/thermal/ThermalUnitInput.java 62.50% <0.00%> (-1.50%) ⬇️
...el/io/source/couchbase/CouchbaseWeatherSource.java 71.76% <0.00%> (-1.41%) ⬇️
...del/models/input/container/JointGridContainer.java 75.00% <0.00%> (-1.20%) ⬇️
...amodel/models/input/graphics/LineGraphicInput.java 83.33% <0.00%> (-0.88%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ebb413...98ae220. Read the comment docs.

ckittl added 13 commits April 9, 2021 10:12
@ckittl
Alter scheme for COSMO weather in test files
697f9da
@ckittl
Renaming of "weather data scheme" and equivalent classes and methods
fa0fc4a
@ckittl
Adapt to plain naming in Couchbase files
15d9550
@ckittl
Fix some tests
343b832
@ckittl
Consolidate csv weather test files in one place
454420d
@ckittl
Use PowerSystemUtils v 1.5.2
5365a79
@ckittl
Use maven central instead of jcenter
c9a8c16
@ckittl
Pull naming of coordinate id {field / key / column} naming into TimeB…
30699e2 
…asedWeatherValueFactory
@ckittl
Fix InfluxDbWeatherSource
b272635
@ckittl
Simplifying the sql weather source by fixing the column name to a giv…
6940eaf 
…en convention
@ckittl
Harmonizing the couchbase weather source with other source implementa…
ccd9418 
…tions
@ckittl
Offer possibility to set naming strategy in sql and couchbase weather…
5609e04 
… sources
@ckittl
Document work
00859be
@ckittl ckittl force-pushed the ck/#267-adaptColumnSchemeOfCOSMOWeatherData branch from efd75ea to 00859be Compare April 9, 2021 08:32
@ckittl ckittl marked this pull request as ready for review April 14, 2021 10:43
@ckittl ckittl mentioned this pull request Apr 16, 2021
Merged
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@ckittl ckittl mentioned this pull request Jan 28, 2022
Closed
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
This was referenced Jan 28, 2022
Open
Merged
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
sebastian-peter
sebastian-peter requested changes Feb 14, 2022
View reviewed changes
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

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

Please furthermore adapt the documentation in io/csvfiles.rst.

src/main/java/edu/ie3/datamodel/io/source/sql/SqlWeatherSource.java
* @param weatherFactory instance of a time based weather value factory
*/
public SqlWeatherSource(
SqlConnector connector,
IdCoordinateSource idCoordinateSource,
String schemaName,
String weatherTableName,
NamingConvention namingConvention,
Copy link
Member

@sebastian-peter sebastian-peter Feb 14, 2022 •
edited

Choose a reason for hiding this comment

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

I think the current condition of this constructor is quite confusing, as the provided namingConvention only applies to the coordinate column name and not the others (such as the values of DIFFUSE_IRRADIANCE and other in IconTimeBasedWeatherValueFactory, which is also not adopted in #329).

I think it'd be best to either use it for all columns of this data type or none, if we introduce such an api change here (this is the first PR to use NamingConvention). In the end, it's probably neater then to introduce the api change in a separate PR, where we can focus on that issue alone.
These changes could be introduced in conjunction with #514.

src/main/java/edu/ie3/datamodel/io/source/couchbase/CouchbaseWeatherSource.java
String keyPrefix,
NamingConvention namingConvention,
Copy link
Member

Choose a reason for hiding this comment

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

Same as in SqlWeatherSource

@sonarqubegithubprchecks

This comment has been minimized.

Sign in to view
@sebastian-peter sebastian-peter marked this pull request as draft March 3, 2022 10:13
@sebastian-peter sebastian-peter mentioned this pull request Mar 3, 2022
Merged
@sebastian-peter
Copy link
Member

As discussed, new PR: #558

@sonarqubegithubprchecks
Copy link

Passed

Analysis Details

2 Issues

  • Bug0 Bugs
  • Vulnerability0 Vulnerabilities
  • Code Smell2 Code Smells

Coverage and Duplications

  • 90 percent coverage93.75% Coverage (77.00% Estimated after merge)
  • 3 percent duplication0.00% Duplicated Code (0.40% Estimated after merge)

Project ID: edu.ie3:PowerSystemDataModel

View in SonarQube

BugTracker automation moved this from High priority to Closed Mar 3, 2022
@sebastian-peter sebastian-peter deleted the ck/#267-adaptColumnSchemeOfCOSMOWeatherData branch July 30, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastian-peter sebastian-peter sebastian-peter requested changes

Assignees

@ckittl ckittl

Labels
bug Something isn't working invalid This doesn't seem right
Projects
BugTracker
  
Closed
Milestone
Version 3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ckittl @sebastian-peter @johanneshiry @t-ober