-
Notifications
You must be signed in to change notification settings - Fork 95
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
Cutout - Access to irradiation data #205
Conversation
…eparately in addition to the total irradiation.
…define what type of irradiation values to return. Defaults to 'total'. This change aims to make the modifications as unobtrusive as possible.
Hi @Tasqu ! Thanks for your PR. I'll have a look at it later this week or next week. |
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.
Thanks for your PR.
Sorry, it took me a while to find the time to sit down and think it through. I have a few comments, some concerning documentation (for others and our future selves), others involve functionality.
Also two remarks on the notebook:
- Can you think of a shorter title for the notebook example? Long titles render badly in the table of contents in the documentation.
- The notebook is not executed, but uploaded without cell output. We try to include meaningful cell output with the examples (figures or display variable values). It'd be great if you could include meaningful cell output with the example, see e.g. this example.
Let me know where you (dis-)agree with my comments.
@euronion I've been a bit busy lately, so I don't know when I'll have the time to address your comments above. Hopefully within a few weeks, but I can't make any promises... Still, thanks for going through my pull request, and for the thorough feedback! |
Great. 👍 No need for promises, As the old saying goes: “Your contributions are never late, nor are they early, they arrive precisely when you meant them to." |
@euronion Ok, I think I've addressed all of your previous comments. I also revised the example so that it doesn't display any unnecessary intermediate output, and included the outputs of interest into the notebook. |
for more information, see https://pre-commit.ci
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.
Review done?
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.
Hey @Tasqu, this is really impressive work, in particular the example. Sorry for taking so long, I'm regularly finding myself postponing reviews of larger PRs.
One point about the example notebook. The projection of the population layout is a bit bloated and probably not fully correct. For the re-projection, could you try something like this:
finland_population_density.rio.reproject(cutout.crs, shape=cutout.shape, transform=cutout.transform)
After that is fixed, I am happy to merge :)
Fixing example .ipynb path for the new example. Co-authored-by: Fabian Hofmann <hofmann@fias.uni-frankfurt.de>
for more information, see https://pre-commit.ci
@FabianHofmann can you clarify which part of the example code you're referring to? In the beginning, I clip the raw European population density raster with finland_population_density = (
population_density.rio.clip(finland_3035.geometry, from_disk=True)
.rio.reproject(cutout.crs, from_disk=True)
.squeeze()
) and I couldn't figure out a way to do the clipping as part of the layout = finland_population_density.rio.reproject(
cutout.crs,
shape=cutout.shape,
transform=cutout.transform,
resampling=5,
from_disk=True
) instead of the |
Hey @Tasqu! Good to hear. It seems that the clipping has to be done either way before the reproject. So you could probably do layout = (
population_density.rio.clip(finland_3035.geometry, from_disk=True)
.rio.reproject(cutout.crs, shape=cutout.shape, transform=cutout.transform, resampling=5, from_disk=True)
.squeeze()
) |
for more information, see https://pre-commit.ci
…atlite into cutout_access_irradiation
Ok, I revised the example to be a bit more dense (and fixed a few typos). I also went through the pull request checklist again, but it seems that Let me know if there's still some stuff you need improved. |
@euronion you have to approve that you are happy with your requested changes now |
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.
Thank you for the changes!
@Tasqu would you be fine if we squash this before merging? |
@FabianHofmann I have no objections, it's your codebase after all 😉 |
great thanks @Tasqu |
Change proposed in this Pull Request
irradiance='total'
toTiltedIrradiance()
to determine whether to return total, direct, diffuse, or ground irradiation. Default assumption is total to preserve the original functionality.convert_irradiation()
andirradiation()
methods to theCutout
class to access the raw irradiance data calculated viaTiltedIrradiance()
.Description
pv/irratiation.py
TiltedIrradiation()
Added a new optional keyword
irradiation="total"
to define what irradiation quantity should be returned. Previously,TiltedIrradiation()
only returned the total irradiation, but for my purposes, I needed access to direct and diffuse irradiation on a tilted surface separately. The method defaults toirradiation="total"
in order to minimise the impact of this change on the rest ofatlite
.convert.py
convert_irradiation()
andirradiation()
Added two new methods for accessing the raw irradiation data,
convert_irradiation()
andirradiation()
. These are basically just reduced versions of theconvert_pv()
andpv()
methods, where we skip the final conversion from solar irradiation to PV generation.cutout.py
irradiation
Added the
irradiation()
method to theCutout
class in order to access the raw irradiation data.examples/finnish_building_stock_weather_aggregation.ipynb
Added a new example to demonstrate using the added features.
examples/finnish_building_stock_weather_aggregation.ipynb.license
License file for the new example,
GPL-3.0-or-later
doc/examples/finnish_building_stock_weather_aggregation.nblink
nblink
file for the new example.doc/index.rst
Tried adding the new example into the documentation, but I couldn't figure out how to build the documentation locally to test this.
Motivation and Context
NOTE! I'm more or less still a beginner when it comes to Python programming, so my code is potentially not the cleanest, nor do I understand anything about generating documentation. For that, I apologise.
I'm working on modelling the heat demand of the Finnish building stock, its potential flexibility, as well as its value in large-scale energy system context. For this reason, I need to be able to calculate historical weather time series over large geographical areas, weighted based on where the majority of the heated volume of the building stock is located. We've used in-house Python codes for these kinds of things previously, but those have fell into disrepair after the original developers have moved on.
Seeing that
atlite
was already capable of doing all the calculations I need, I decided it would be easier to tweakatlite
, than to try and learn and repair our old in-house codes. However, the currentatlite
master
doesn't actually contain methods for accessing the tilted solar irradiation directly, as it is only used for PV and solar thermal conversion. Thus, I added the necessary methods myself.How Has This Been Tested?
So far, mostly just using the examples. The new one works as intended, and I don't think I broke any of the old ones.
Also run the
pytest
as suggested by the checklist, and nothing alarming seemed to come up. (Although, I'm still a beginner with Python so I'm not sure I would necessarily recognise potential issues)Type of change
Checklist
pytest
inside the repository and no unexpected problems came up.doc/
. (I tried adding the new example to the documentation, but don't know how to test building the documentation locally.)environment.yaml
file. (no new dependencies)doc/release_notes.rst
. (RELEASE_NOTES.rst
ordoc/release_notes.rst
? Thedoc
version seems a bit weird for editing...)pre-commit run --all
to lint/format/check my contribution