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
Draft
wants to merge 15 commits into
base: dev
Choose a base branch
from
Draft

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

wants to merge 15 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.

src/main/java/edu/ie3/datamodel/io/factory/timeseries/CosmoTimeBasedWeatherValueFactory.java
Comment on lines +65 to +67
Set<String> withGroundTemp = expandSet(minConstructorParams, GROUND_TEMPERATURE);

return List.of(minConstructorParams, withGroundTemp);
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if there are no ground temperatures and one calls this method?

src/main/java/edu/ie3/datamodel/io/factory/timeseries/CosmoTimeBasedWeatherValueFactory.java
Comment on lines +32 to +33
private static final Logger logger =
LoggerFactory.getLogger(CosmoTimeBasedWeatherValueFactory.class);
Copy link
Member

Choose a reason for hiding this comment

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

What does logger log in this class?

src/main/java/edu/ie3/datamodel/models/value/GroundTemperatureValue.java
import tech.units.indriya.ComparableQuantity;

/** Describes a ground temperature value. */
public class GroundTemperatureValue implements Value {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can reuse many parts from TemperatureValue if you make GroundTemperatureValue extending from it.

src/main/java/edu/ie3/datamodel/io/factory/timeseries/IconTimeBasedWeatherValueFactory.java
Comment on lines +100 to +106
ComparableQuantity<Temperature> groundTemperature = null;
try {
groundTemperature =
data.getQuantity(GROUND_TEMPERATURE, Units.KELVIN).to(StandardUnits.TEMPERATURE);
} catch (IllegalArgumentException ignored) {
}

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 for cosmo

src/main/java/edu/ie3/datamodel/io/factory/timeseries/IconTimeBasedWeatherValueFactory.java
Comment on lines +33 to +34
private static final Logger logger =
LoggerFactory.getLogger(IconTimeBasedWeatherValueFactory.class);
Copy link
Member

Choose a reason for hiding this comment

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

Same for logger here

src/main/java/edu/ie3/datamodel/io/factory/timeseries/CosmoTimeBasedWeatherValueFactory.java
Comment on lines +85 to +90
ComparableQuantity<Temperature> groundTemperature = null;
try {
groundTemperature = data.getQuantity(GROUND_TEMPERATURE, StandardUnits.TEMPERATURE);
} catch (IllegalArgumentException ignored) {

}
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion this is a bit counterintuitive. The advantage of try is that one get's a result of the try. But we ignore this.

But first, please change to Optional<ComparableQuantity<Tempearture>> . And then it should be possible to try to get the quantity from data, as for the others, and wrap it into some Optional.ofNullable, or similar.

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
2 participants
@pierrepetersmeier @danielfeismann