Implement various improvements to VDI feature

[jnettels]

I found several possible improvements for the VDI4655 feature. I considered opening a PR for each of them, but I think that would be too much effort to review. The commits are cleanly separated, except for the changes to the documentation (which is affected by most of the changes).

• Make parameters 'summer_temperature_limit' and 'winter_temperature_limit' actually optional

• Allow skipping any energy type: Heating, hot water and electrical demand are now optional parameters. If they are not provided, the respective time series will be returned with all NaNs.

• Allow custom DWD weather data by exposing a new function from_file(), which the existing from_try_data() now uses internally

• Remove unused parameter 'copies' from example and reorder parameters

• Adapt VDI documentation to latest changes

I hope this is fine, if necessary I will separate e.g. the weather data commit into a new PR.

[uvchik]

Thank you for improving the documentation.

The possibility to upload other weather data is okay for me. What is important to me is that matching energy factors must be uploaded at the same time so that users realise that the two are directly related. You can't upload new weather data if you don't have the matching energy factors.

A dictionary is already a problematic construct for providing input data. If other problems arise now, we should make a separate building class out of it.

[uvchik]

I do not agree with this behaviour. This will also happen if there is a typo in the parameter. I think the best way to deal with this ist to introduce a Building() class where we can define what will happen with any of the parameters.

[uvchik]

See acomment above.

[uvchik]

See comment above.

[jnettels]

Thanks for your comments. I did not realize this would be so controversial, so maybe it would be better to remove this PR and create one for "custom weather data" and one for "updated building options" (and a third to update the example and docs after both are merged). Would you prefer that?

In the mean time we should separate the following two discussions:

Custom weather data

I guess your sentiment is that by using custom weather data we leave the defined use case of the norm? The energy factor matrix was fitted to each of the 15 weather regions and weather data specifically. Combining custom weather data with the existing energy factors can be considered inappropriate. Requiring users to provide energy factors that match their weather data is the logical conclusion.
However, I do not know how to generate those energy factors. From what I can read, the norm does not give any indication how those were actually generated. I assume it is a part of fitting real measurement data into the 'typical day' style. A user who is able to replicate this does not need the norm.

Please correct me, but I only see two outcomes:

1. A user cannot create energy factors: This prevents them from using custom weather data
2. A user digs into the source code to see how the official energy factors are loaded and writes their own function for creating a Climate object with the daily mean values of their weather data and the official energy factors

I think it should be each user's responsibility to decide weather they stick to the norm and the official weather data, or prefer using their own weather data at the cost of some minor "error" in the way the typical days are scaled relative to each other. Using the wrong weather data might be the worse decision in many cases.

My proposed function Climate().from_file() just puts an easy to use wrapper around the option 2 above, instead of forcing users to implement it themselves. What I should definitely improve is to add a note or warning to the function docstring to explain that it is not part of the official VDI usage.

Building options

You raise a valid point and I totally agree that it would be annoying to never notice that a setting for e.g. summer_temperature_limit was not in effect because of a typo.
Region().add_houses() already performs a check for valid house types (EFH, MFH). It would be straightforward to also test the houses for invalid arguments. Don't you think this would be far simpler to implement than switching to a new logic with a Building class? In any case there should always be a convenience function to create all the houses from e.g. a DataFrame or a list of dicts, where those tests for valid arguments would be necessary anyway.

I can offer to implement a check in Region().add_houses(), but I am not sure if I can write a class implementation that matches your vision.

[uvchik]

Custom weather data

We could make the method private, making it clear that you should not use it. In fact, in Python you can use it anyway:

def _from_file(self, fn_weather, try_region, hoy=8760): 
    pass

However, you expect the user to have a very specific weather file. Most users will have csv files. Your method suggests that it is possible to load custom weather files, but in fact you can only load a very specific formatted weather file. Why not leave it up to the user. Anyone is free to use the following function:

weather = dwd_try.read_dwd_weather_file(fn_weather)

Don't you think that people will be able to handle their own weather data and extract the temperature and cloud covererage?

There are no other energy factors at the moment, but there may be in the future. People just have to add three parameters and if they know what they are doing, they can just do it. A convenient method that returns a wrong combination does not seem useful to me.

Building options

In the first step you can see the Building() class as a container, which is nothing more than a dictionary with given keys. In a second step you could add checks or methods if needed. In the docstring of this class you can clearly discribe the parameters. That makes it easier to use than discribing a dictionary that has to have to following keys... In a class you could even check the input if you want.
Furthermore, you could add the option to implement a MultiHouse that works like a singel hous but each parameter is a Series a not just a single value.

[jnettels]

I implemented some changes as suggestions.

Custom weather data

• I renamed the function to from_dwd_weather_file() to make the required file type obvious (you raised a valid point there)
• I added quite a bit of notes to the docstring and the documentation to clarify that this is not the usage intended by the norm
• I considered marking it as private _from_dwd_weather_file(), but it felt wrong to then mention it in the documentation, which I would still prefer

But all of this may not solve the fundamental problem:

Don't you think that people will be able to handle their own weather data and extract the temperature and cloud covererage?

I do expect users to be able to handle their own weather data and extract the temperature and cloud coverage. But that is beside the point, because I cannot expect them to come up with their own energy factors. This prohibits the usage of custom weather data.

Let's look at it this way: In the projects I am involved, it is quite common to use the current DWD TRY weather data. On the other hand, the 2010 DWD weather should arguably not be used anymore, because it is out of date and even contains errors (specifically in the radiation data). Also, no one would want use different weather data for different parts of a project. Also I think it is fair to assume that most users who want to use VDI profiles are from Germany.
So it seems obvious to provide a convenience function for what is, from my point of view, the primary way to use this implementation. Of course, anyone with enough time and knowledge could implement what is now from_dwd_weather_file() for themselves, but I would prefer to have a well documented function that explains the caveats.
Also we are still talking about standardized profiles. They are never "correct" and always a rough estimation for cases where you cannot use e.g. measurement data or simulated load profiles. That is why I think the "error" introduced by using the "wrong" energy factors can be acceptable, if you are aware of them.

If you cannot agree with that, would making the function private be an acceptable compromise? Would we have to remove it from the documentation then?

Building options

As a first step, I added checks for required and unsupported parameters to add_houses(). This is what I consider the actually required functionality. Can we agree on that?
Moving it to a new class could become a new step when I find more time, maybe in a separate PR?

[uvchik]

Isn't it just easy to set a specific Parameter to None or 0?

my_houses.append(
        {
            "N_Pers": 3,
            "name": "EFH_{0}".format(n),
            "N_WE": 1,
            "Q_Heiz_a": 6000,
            "copies": 24,
            "house_type": "EFH",
            "Q_TWW_a": 1500,
            "W_a": None,
            "summer_temperature_limit": 15,
            "winter_temperature_limit": 5,
        }
    )

When I find time, I could add a Building class. Actually there is already a building class and we should bring both approaches as close together as possible.

Adding your own weather data and using the VDI energy factors is just wrong and I would not provide a function to do wrong things. If you want we could internally provide a def _load_weather_file(fn_weather, hoy) and def _load_energy_factors(try_region). In that case you could use these two functions to combine weather files with any data row of the energy factors but I would not announce that in the documentation or in the docstring.

[uvchik]

BTW: It might save time if you do not implent everthing diretly but first discuss what you want to do. It feels strange to me if you provide many lines of code and I have to say, that I do not agree with the general approach.

[p-snft]

Thank you for your effort. Honestly, I am not really deep into standard heat demand curves, however, I can comment this from a software design perspective.

To hopefully solve the discussion on custom weather: We currently allow to define custom seasons in the BDEW SLP, which is also not to standard. Thus, it might be consistent to also allow some more freedom here. Also, we have the class vdi.Region(temperature, cloud_coverage, energy_factors), which already allows to give user-defined energy factors.

However, if the standard defines specific regions and according TRYs, we should explicitly state that it's not up to standard if these numbers are changed. (The same is true for the BDEW electricity SLPs from 2000. We should state that "custom seasons" violate the standard, if they do.)

[p-snft]

I would always require these inputs. If you do not want to have TWW, you can simply give 0.

[p-snft]

Aren't these temperatures in the norm? If so, I think it's okay to set them as defaults.

[jnettels]

Correct, these are the values used per default in the norm.

[jnettels]

It might save time if you do not implent everthing diretly

It makes me think my suggestions through and allows me to test them. Also I feel like sometimes just writing the code is the most precise form of communicating a suggestion.

Custom weather data

I think I cannot add anything new to the discussion. Thanks for chiming in, @p-snft. You ask for a specific disclaimer in case of deviations from the standard. This is the disclaimer I already included in the function from_dwd_weather_file(). Does it make sense to you?

The function from_try_data() is the implementation for using
weather data as intended by the VDI 4655, because it loads
the original weather data. Using different weather data is
not supported by the norm.
However, from_dwd_weather_file() enables users to load the
most recent DWD test reference year weather files at their
own risk. They still need to provide a TRY region number,
which is required for loading the energy factors.
These are used for scaling the typical days relative to each
other, depending on the TRY region. But users need to be aware
that they do not have the originally intended effect when
used with different weather data.
Other file types are currently not supported. Instead, users
need to create a Climate() object and provide temperature
and cloud coverage time series, as well as matching energy
factors.

To answer this comment:

If you want we could internally provide a def _load_weather_file(fn_weather, hoy) and def _load_energy_factors(try_region).

If we cannot agree on anything else, this would be a useful addition, yes.

Building options

You both agree that all three energy values should be required arguments.

But I'd like to make some distinctions: None and 0 are not the same input. 0 causes a profile with a constant value of 0, which is still a profile, strictly speaking. Returning a series of all NaN is communicating that there is no profile.
I actually ran into a use case where I needed to create profiles for a set of buildings. Some with useful annual values, some with a value of 0 and some with missing values. I want all of these to be valid inputs and be reflected in the results. I don't want the function to change the meaning of the input.

Allowing NaN can simply also be necessary for saving memory/storage space when working with large sets of buildings (instead of endless columns of 0).

If None is allowed, then it might as well be the default value for an optional parameter. If you just imagine an arbitrary function call like this (or a similar constructor of a class):
make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000)
vs.
make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000, W_a=None)
If I do not care for the electricity profile, why should I have to specify it? I feel like this is not how functions are usually designed... is that just me?

[p-snft]

Allowing NaN can simply also be necessary for saving memory/storage space when working with large sets of buildings (instead of endless columns of 0).
If None is allowed, then it might as well be the default value for an optional parameter. If you just imagine an arbitrary function call like this (or a similar constructor of a class):
make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000)
vs. make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000, W_a=None)
If I do not care for the electricity profile, why should I have to specify it? I feel like this is not how functions are usually designed... is that just me?

Now, I see the point why you want to have the values undefined. However, creating garbage data (NaN) would not be the expected result of that. You will have endless columns of NaN which isn't any better than 0. It consumes the same memory and even computations are executed. When I put None as an input, I would expect None as an output, not NaN.

It makes me think my suggestions through and allows me to test them. Also I feel like sometimes just writing the code is the most precise form of communicating a suggestion.

I feel the same. For bigger changes I try to draft and discuss them beforehand, but sometimes an implementation is the fastest way specify something. (Following this approach, however, also means that suggested changes will be rejected, as implementation and rejection are part of the discussion process.)

[uvchik]

It makes me think my suggestions through and allows me to test them. Also I feel like sometimes just writing the code is the most precise form of communicating a suggestion.

I just didn't want you to write a lot of code that might get deleted, but if that's okay with you, it's okay with me.

To hopefully solve the discussion on custom weather: We currently allow to define custom seasons in the BDEW SLP, which is also not to standard. Thus, it might be consistent to also allow some more freedom here.

I would go the other way round and change the API for BDEW SLP.

1. Just to be clear: I do not want to forbid that, I just do not want to provide an official API. People who know what they are doing can do whatever they want.
2. A lot of disclaimer are for legal reasons: "We think it is fine, but we have to add this disclaimer". In our case we combine energy-factors that a exclusivly made for a specific wheather data set with a new data set. Can be right, can be wrong. Nobody knows. But people will have the feeling that it is more regional, because they use the new TRY data which is available on a 1x1km raster.
3. It is still my opinion but if it ends up with a 2:1 I will not block it.

If None is allowed, then it might as well be the default value for an optional parameter. If you just imagine an arbitrary function call like this (or a similar constructor of a class):
make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000)
vs.
make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000, W_a=None)
If I do not care for the electricity profile, why should I have to specify it? I feel like this is not how functions are usually designed... is that just me?

This is missleading. If you have a method as described above it is totally fine, but this is not what we have here. We have something like this:
make_me_some_profiles_plz(my_dict)
And my_dict is something like this:

my_dict= {
            "N_Pers": 3,
            "name": "EFH_{0}".format(n),
            "N_WE": 1,
            "Q_Heiz_a": 6000,
            "copies": 24,
            "house_type": "EFH",
            "Q_TWW_a": 1500,
            "W_a": None,
            "summer_temperature_limit": 15,
            "winter_temperature_limit": 5,
        }
    )

make_me_some_profiles_plz(Q_Heiz_a=5000, Q_TWW_a=1000, Wa=2000) would raise an error but make_me_some_profiles_plz({Q_Heiz_a=5000, Q_TWW_a=1000, Wa=2000}) would not raise an error and will silently use W_a=None. A solution is to add a minimal building-class. In a building class one could define the __init__-method:

class Building:
   def __init__(self, q_heiz_a=None, q_tww_a=None, w_a=None):
       self.q_heiz_a = q_heiz_a
       self.q_tww_a = q_tww_a
       self.w_a = w_a

The code would look like this:

my_building = Building(q_heiz_a=5000, q_tww_a=1000, w_a=2000)
make_me_some_profiles_plz(my_building)

Now it works as @jnettels intended and my_building = Building(q_heiz_a=5000, q_tww_a=1000, wa=2000) would raise an error.

EDIT: It does not have to be a Building class it could also be a Region class or whatever name.