-
Notifications
You must be signed in to change notification settings - Fork 78
Add initial versions for charts leading into plotting functionality #161
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.
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
base: development
Are you sure you want to change the base?
Conversation
pythermalcomfort/charts.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shoudl be an input and maybe three separate variables.
|
@t-kramer great work and thank you for contributing. You have done a great work and I am very happy to see the charts implemented in pythermalcomfort. I have a few comments below and inline in the code in the pull request. code
chartsthe chart look great and very modern.
motivation
agree also with the following comment, the bar chart summary is essential, the heatmap alone is beautiful but useless. We should also consider different grouping, month, time, etc.
next step
agree, we should probably do not include it with dot notation. |
This commit addresses the comments from PR #161 reviewed by @FedericoTartarini. The following changes were made: - improved and added summary for all charts - created new modules for charts in pythermalcomfort/charts - replaced chart_development.ipynb with example scripts and move these to pythermalcomfort/examples - move theme.py to pythermalcomfort/charts - additional minor fixes
|
@FedericoTartarini I finished most of my work yesterday, but didn't have the time to update the PR, so here it comes. AdjustmentsI implemented most of your requested changes.
More information below and in the responses to your inline comments. Charts
This was another way of summarizing the data based on this paper and Stefano's suggestion. The outer circles represent 100% and the inner circles the actually achieved number. Anyway, I removed this style and added the horizontal bar chart summary for all (see screenshot).
Agree on all of these points, changes have been implemented accordingly. Here are the updated charts (all including the summary, which can be hidden):
I know, the bar chart for the UTCI seems a bit crowded. Need to find a way how to handle this in a flexible way. Also, the missing data points (white pixels) are those that are out of range and therefore NaN. Further, I do not agree completely with this comment:
While the heatmap alone can't give you a detailed summary of the whole year, it is very helpful in pointing out specific design problems when plotting indoor environmental data, e.g. too much solar radiation causing overheating at very specific hours of the year. So for some cases, people might want to use the heatmap without the summary. Next Steps
Let's do it! 🚀 Thanks again for providing the detailed review, @FedericoTartarini! ❤️ |
|
Great work. I left a few more comments in line with the code. Below a response to some of your comments and a few observations about the chart. I love how the charts look.
Thank you so much for working on this. I really appreciate your help. I have noticed that I can no longer see all my all comments, however, only a few of them have not been addressed yet. |
FedericoTartarini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed most of the code, and I left a few comments in line. Thank you so much for working on this.
One thing that a little bit worries me is that the some of my old comments are lost, but they were not addressed in this commit. Do you know what may have happened? We can discuss the code the during the upcoming meeting.
| import sys | ||
| import os | ||
|
|
||
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this code? can we remove it or find a more elegant way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the first four lines can go eventually. I'm just having some problem with the system path, maybe we can resolve it together. You used something similar in docs/conf.py, maybe we can fix this issue globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, let's try to fix it globally since it does not look very nice here at the beginning of the example
| low_memory=False, | ||
| ).reset_index(drop=True) | ||
|
|
||
| adaptive_subset = ashrae_measurements[["ta", "tr", "t_out", "vel"]].dropna() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we also add some text in which we explain the source data and a df.head() in which we preview a few data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. We do you want to add this, here or within the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here since this is only relevant to the example, is not it? In the function documentation we need to state if we want a dataframe as input with or without nan
| A DataFrame containing the input data. Must have the following columns: | ||
| 'ta' (Dry bulb temperature [°C]), 'rh' (Relative humidity [%]). | ||
| pmv_constants : dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a huge fan of dictionaries and dictionaries as inputs
|
|
||
| # helper functions start | ||
|
|
||
| def pmv_eq(T, RH): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pythons to the variable name should not be capitalized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this function to be defined here? can't we just have the PMV function in line 84?
| [[convert_rh_to_hr(T, RH) for T in t_dry] for RH in rh_levels] | ||
| ) | ||
|
|
||
| target_pmvs = {"Neutral": 0, "Upper": 0.5, "Lower": -0.5} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an input
|
|
||
| tight_margins = dict(l=35, r=35, t=45, b=20) | ||
|
|
||
| index_mapping_dictionary = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be using dictionary about the data classes instead
|
@FedericoTartarini Thanks again for a great review! Most of the comments are super clear and will be easy to implement. There are a few where I'd like to learn more and understand why you prefer to avoid certain things, but I'll ask for more detail in my response to the comments. 👍🏼 I will try to continue working on this and implement most requested changes before Thursday (PT). Regarding the comments you can't find, can any of these be the reasons?
Anyway, I did not delete any on purpose and will make sure that for the next changes, that I am a bit more careful in dealing with the comments. Whats your usual workflow? Do you respond to the comments in IDE or GitHub? Do respond to them individually and directly after addressing them or at the end, just before pushing? |
|
@t-kramer thank you for dedicating sometime to work on this before the meeting. Don't worry about the comments. I think they are all there. Perhaps I forgot to add a few last time. You did an amazing job replying to my comments, so don't worry. Sorry, sometimes for being annoying and asking you to change some stuff, but I just want to make sure that the code is easy to read and consistent. I generally have the comments from GitHub, simply because I am lazy, and I've never learned how to do it inside by PyCharm. I am actually feeling embarrassed about this as I am typing the message. This is my new year resolution :) |
|
@t-kramer I also forgot to mention that it would be great if you could extract the thermal comfort function from the plotting function. For example, considering that we have a different PMV model, we don't want to have a chart function for each specific formulation. It is better that the user passes in a column with the PMV results and then after the code does the plotting. |
Merge was necessary to update the plotting-feature branch with the latest changes from the upstream development branch
|
@t-kramer thank you for the great discussion today. It is always great to chat with you. Below is code snippet of the dataclasses. Inside the data class so we can also embed functions if we want for example to return the colors of all the risk categories |
FedericoTartarini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the comments I wrote during the meeting
pythermalcomfort/charts/pmv_chart.py
Outdated
| ) | ||
|
|
||
|
|
||
| def pmv_chart(df, pmv_constants, show_summary=False, si_ip="si"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be called psychometric and should take as input t, rh, pmv
| 'tr' (Mean radiant temperature [°C]), 'v' (Air speed [m/s]), 'met' (Metabolic rate [met]), 'clo' (Clothing | ||
| insulation [clo]). | ||
| show_summary : bool, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it since it will be in a separate function
| show_summary : bool, optional | ||
| If True, a summary of the data is shown. Default is False. | ||
| si_ip : str, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
units
| var_metadata["order_categories"], | ||
| ) | ||
|
|
||
| t_dry = np.linspace(10, 36, 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input default 10, 36
| ) | ||
|
|
||
| t_dry = np.linspace(10, 36, 500) | ||
| rh_levels = np.linspace(0, 100, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input
|
|
||
| ### helper functions end | ||
|
|
||
| humidity_ratios = np.array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use itertool to combine t and rh in all the combinations
| showlegend=False, | ||
| hovertemplate="t_dry: %{x:.1f}°C<br>HR: %{y:.2f}<br><extra></extra>", | ||
| ), | ||
| row=1 if show_summary else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a code repetition
| fig.add_trace( | ||
| go.Scatter( | ||
| x=neutral_line[0], | ||
| y=neutral_line[1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this I don't like
|
|
||
| np.seterr(divide="ignore", invalid="ignore") | ||
|
|
||
| def extract_comfort_data(zone): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write the return type hint
|
|
||
| fig.add_trace( | ||
| go.Scatter( | ||
| x=np.concatenate([upper_line[0], lower_line[0][::-1]]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this array is lenght of 4 and it alternated x1, y1, x2, y2
'origin/development' into plotting-feature
Commit covers a first implementation of dataclasses for adaptive charts. While not completely implemented, I think the idea is clear. Based on review we can implement a similar structure for the other charts. The following additional, minor changes were made: - implemented return of stress_category for heat_index_rothfusz - added >= 27 °C criterion for heat_index_rothfusz, returns NaN if not met - adjusted plot_heatmap_hi accordingly - slight modifications to application of comfort models to dataframes: conversion to np.array to longer needed
|
@FedericoTartarini I just pushed my latest changes and will share a video link shortly to explain them in more detail (avoiding an overly long description here). Summary of Changes:
Especially on the first two points, I'd like you to review before I continue implementing the some structure for the other charts. Next Steps:Most open comments will be resolved once dataclasses are fully implemented across all functions. However, a few items still need clarification/discussion/additional changes:
Let me know your thoughts on these points and how you'd like to proceed! |
|
Great idea recording the video and great work. Brief comments are below:
Here is the video I recorded. |
|
@FedericoTartarini Thanks for the feedback. I'll keep working on it. Quick point: I did push the code yesterday before updating this PR, so you should be able to see the changes? Please check again. |
|
sorry, I think I have heard in the video that you mentioned that you did not push the code so I did not check the pull request. Please let me know if my comments make sense and please let me know if you do not agree with them. |
This commit addresses the comments from PR CenterForTheBuiltEnvironment#161 reviewed by @FedericoTartarini. The following changes were made: - improved and added summary for all charts - created new modules for charts in pythermalcomfort/charts - replaced chart_development.ipynb with example scripts and move these to pythermalcomfort/examples - move theme.py to pythermalcomfort/charts - additional minor fixes
|
Suggestions to improve the graphs for the adaptive thermal comfort model chart:
Suggestions to improve the graphs for the PMV psychrometric model chart:
Suggestions to improve the graphs for the HI:
For the UTCI and HI index show the hours every 3 o 6 hours, not every 2. |
|
Thank you @stefanoschiavon for the comments. We will incorporate them in the new plotting functions. However, I was hoping to get your feedback on this discussion #252 not on this pull request. |
|
@FedericoTartarini, yes I know that I have to work on 252, I will do it on Monday. I have also promised to Toby to review his pull, so he can finalize the "educational" examples that will not need to be implemented in the code but could be there as good examples. |







PR introduces preliminary versions for a first set of charts that can be used along and/or integrated with pythermalcomfort. Before we improve the charts, clean the code and add more, we should discuss how we want to integrate such plots in general. See more on this below in the Motivation bit.
Changes
charts.py: contains initial code for an Adaptive chart (+ alternative version with timeseries summary), PMV chart, and a Heatmap (again with a summary option)theme.py: defines a Plotly template, enhancing consistency and streamlining plot functions - very basic for now, but can be leveraged much morechart_development.ipynb: loads and preprocesses datasets used as input for charts (to be removed later)Added dependencies
Screenshots
Images of charts created using Plotly: Most of them are based of code that I've written earlier and some functions that we used for Clima and the new Comfort Tool version. All plots are interactive, so they provide additional information when hovering over them, e.g. exact x, y, z data behind plotted data points.
Motivation / Ideas behind the charts
big strength of pythermalcomfort over the CBE Comfort Tool: people can use it with multiple samples of input data instead of just one configuration
makes the package very interesting for energy modelers and other AEC people, e.g. analyzing time series data
plots should focus on these applications
in pythermalcomfort, we have mainly three types of functions: PMV-based indexes, Adaptive indexes and others who either generate a temperature-based output or 'comfort' category (e.g. Heat Index)
for a start, we could create one plot for each of these types: summarize time-series data, e.g. hourly simulation or measurement results and longitudinal field study data
heatmap is very flexible: allows to plot the data (continuous or categorical) for most of the indices we offer
added version with a little subplot on the right: summary is important for AEC people, especially when comparing different datasets - visually, this is often not clear enough and needs to be backed by some numbers, e.g. in per cent or hour counts
can add something similar to the PMV plot, too
(Potential) Next Steps
Up for discussion - How do we want to integrate this?
Initially, I was leaning towards integrating the plotting functionality as much as possible with the main codebase, for example using dot notation. However, I changed by mind a bit. I think especially when it comes to plotting, we should grant lots of flexibility. Pythermalcomfort is great, because it simplifies peoples workflows significantly (since they don't have to write all the included functions themselves), but also offer lots and lots of flexibility on how to use these functions.
So for everything that goes beyond the main codebase and it's functions, e.g. the plots, we could just provide a few examples of how to use pythermalcomfort on typical datasets in Pandas dataframe format (the examples don't do that yet, although I think it's the main use case) and provide users with a few insightful and nicely formatted plots, wrapped in functions they can manipulate easily if they want. Internally, we can keep curating these plots and update them on a rolling basis, for example when we create new plots using pythermalcomfort for data processing for papers or our tools. Considering the variety of applications of pythermalcomfort, writing some highly specific plots will limit their application in practice.
@FedericoTartarini @stefanoschiavon
Keen to hear your feedback and thoughts. 🙋🏼♂️
\T