-
Notifications
You must be signed in to change notification settings - Fork 173
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
Natura tiff check #454
Natura tiff check #454
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
An initial idea of the proposed fix implied that AfricaCentral AsiaIt appears that boundaries of the First, the shape is defined like that Then it's reversed when using it when specifying raster Changing the order in the shape definition seems to fix the issue The cutout- However, I'm not quite sure that have completely understood the logic behind the raster transformation. @davide-f, @pz-max, would be very grateful if you could please have a look on this. |
@ekatef thanks for this amazing report 🥇 Yes, a check of all of the blue area is covered by orange would be great. Ideally, the nature raster should match the cutout which seemingly did not work. You fix improved things but it seems there is some unnecessary stuff happening? I think the aim is to have a little buffer/ more of the natura area compared to the cutout (=orange being slightly larger than blue). Regarding the raster transformation, I am also not quite sure maybe @euronion has an idea |
Many thanks, @pz-max! If I understand properly, a safety margin is guaranteed by the So, both the raster dimensions are on one What I don't understand is why the left upper corner of the cutout and the natura-raster do not match each other for Africa. According to raster generation in |
Yes, it raises question marks. Let's discuss that tomorrow at the weekly. Super interesting and important issue anyways. Thanks for spotting this 🥇 |
Update: it looks like that the reason of the shape discrepancy between the It seems |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Better to direct questions regarding the transform to @FabianHofmann . In general note that with newer |
# x: right-left, y: top-bottom | ||
shape = int((right - left) // res), int((top - bottom) // res) |
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.
@ekatef are you sure about switching the order of x
y
? Normally in GIS applications, the y
axis is the first dimension (as in a matrix) and x
the second which is indeed confusing.
@euronion, many thanks for looking into it! Good to know about changes in the |
@FabianHofmann, many thanks for the review. I have been trying to keep the logic of the original code where a coordinates swap is applied to the If I understand properly, the intention behind this approach was to have a regular cartesian coordinates order (x, y) in Generally, absolutely agree that it's always worth to be aware of "x for longitude, y for latitude" when working with coordinates for GIS-related problems... |
The testing procedure for an arbitrary region may be reproduces as follows:
@pz-max, will be very grateful for your review :) |
Awesome @ekatef ! Regarding the question you ose to Fabian, I believe keeping the lat-lon convenction coherent in the script should be the way to go |
@ekatef , can we solve this today? To my understanding from the weekly meeting, I could just review this PR as it appears to work already. |
@davide-f, thank you so much for looking into it! I'll check all the points you have raised. @pz-max, not sure I'll be able to finalize it today. I'd expect that some additional work with data will be needed to double-check the proposed approach. But it'll be not difficult even to re-create this PR if needed as the most work here is work with data not code. So, if you are intended to rebrand today I think it absolutely makes sense to proceed with it :) |
@ekatef thanks for the feedback. Keep your local changes to be contributed from a fresh clone in future. |
@ekatef , rebranding is done. Feel free to work on the PR from a fresh clone or even fresh fork. |
@pz-max, many thanks for the great news :) Amazing work! Regarding the new fork: it looks like I need to remove the previous fork to create a new one, right? |
If you do a clone from the fork.. How large is the repo? |
A fresh clone from a fresh fork in my case is 3.1MB. |
Even better :) |
Closes #442 .
Changes proposed in this Pull Request
Discrepancy between the cutout and natura.tiff extents can lead to errors when generating renewable profiles. This PR is intended to prevent this by adding a check to ensure that natura.tiff fully covers the cutout area and was generated using a proper CRS.
Checklist
envs/environment.yaml
andenvs/environment.docs.yaml
.config.default.yaml
,config.tutorial.yaml
, andtest/config.test1.yaml
.doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.