-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add wrf version #127
base: main
Are you sure you want to change the base?
Add wrf version #127
Conversation
setup.cfg
Outdated
@@ -18,6 +18,7 @@ classifiers = | |||
[options] | |||
packages = find: | |||
install_requires = | |||
bottleneck>=1.3.4 |
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.
why do we need this? I don't think we're using that anywhere?
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.
pandas was complaining that this version of bottleneck was required. Hence this addition.
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.
Mhm, this seems to be only an optional dependency from pandas:
https://github.com/pandas-dev/pandas/blob/f5d754d4fcaefff9ff08167a55426f3afe88b175/pyproject.toml#L67
did you try this in a clean environment? I works fine for me without it...
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.
Ha, yes, you are right ... this can indeed be removed then!
# WRF version dict | ||
WRF_VERSIONS_DICT = { | ||
'v4.3': {'ADD_LCZ_INT': 30, 'NUM_LAND_CAT': 41}, | ||
'v4.3.1': {'ADD_LCZ_INT': 30, 'NUM_LAND_CAT': 41}, | ||
'v4.3.2': {'ADD_LCZ_INT': 30, 'NUM_LAND_CAT': 41}, | ||
'v4.3.3': {'ADD_LCZ_INT': 30, 'NUM_LAND_CAT': 41}, | ||
'v4.4': {'ADD_LCZ_INT': 30, 'NUM_LAND_CAT': 41}, | ||
'v4.4.1': {'ADD_LCZ_INT': 30, 'NUM_LAND_CAT': 41}, | ||
'v4.4.2': {'ADD_LCZ_INT': 50, 'NUM_LAND_CAT': 61}, | ||
'v4.5': {'ADD_LCZ_INT': 50, 'NUM_LAND_CAT': 61}, | ||
'v4.5.1': {'ADD_LCZ_INT': 50, 'NUM_LAND_CAT': 61}, | ||
'v4.5.2': {'ADD_LCZ_INT': 50, 'NUM_LAND_CAT': 61}, | ||
} | ||
|
||
|
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.
let's have the dict in a single location and add new versions only here. This automatically populates the argument validation.
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 missed this comment/question. What do you mean by this? You want to have this in a separate file? Or ...?
It is probably not a bad idea, since this dict probably needs updates whenever a new WRF version is released.
Even though the next WRF release will have W2W integrated into the WPS/WRF system itself ...
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.
Nah, just as a module-level constant as it currently is so in one place only. This way, changing things in that dict will automatically change e.g. the help texts, the valid choices etc.
This comment was basically just me explaining why I moved that.
Yes! Defininetely makes sense. Thanks @matthiasdemuzere ! I'l try it tomorrow |
Thanks for reviewing @jkittner. I see now that all tests pass. That is great. |
oh of course - I always forget about docs 🤣 |
Also tested locally on my end with my custom data and v4.5.2, it works fine as well. Outputs look correct. |
Hmmm the truth is that the new version doesn't calculate the NoUrban well, my guess is that it is looking for the previous categories. I'll need to fix that. It probably broke in previous versions, not this PR. I'll look at it. |
Mmmm, what is the LU_INDEX you are having in this file? |
So the attribute ISURBAN still 13 despite the fact that urban areas are actually categories 50-60. This means that the code looks for LU_INDEX that matches ISURBAN and replace them, but there are none and the urban areas are still there with there original category in the range 50-60. I've got this fixed now and will commit to this PR once I've tested it. However, the resulting file does not work with my setup in which I have 3 domains, a larger one and then 2 identical domains, except for the urban areas. The file with LCZ_params doesn't work either and it looks like its related to GREENFRAC |
I've also fixed the issue with the GREENFRAC. The problem was that the urban areas were also identified with ISURBAN to replace GREENFRAC. Now it will check the number of categories and select urban areas including LCZ categories too. |
Hi @dargueso, thanks for your feedback on this. Thus far I did not consider the fact that users might feed W2W a geo_em file that already contains LCZ labels, numbered from 50-61, and derived from the MODIS-CGLC-LCZ dataset. This makes things even more confusing. What I'd like to suggest:
This starts to become a bit of a circular behaviour, but at the moment I do not see another solution? So, I can try to add this, and then hopefully you can test again (with a WRF version that understand labels 30..41 and labels 50..61)? M. |
I’ve done that already, it does something similar to what you suggest, except that those changes are implemented only where the code checks ISURBAN.
I can add the changes to your pull request, or create a new pull request, but I’m inheriting your pull request anyway because I built on it.
On 18 Mar 2024, at 14:53, Matthias Demuzere ***@***.***> wrote:
Hi @dargueso<https://urldefense.com/v3/__https://github.com/dargueso__;!!D9dNQwwGXtA!SfYJ9mqClESjywhHvYKXq8q8HEOybxxNa10Lpy_YiYvG4Pcy3dj_mu3j-JbIuXgHSVfZ-efxEA8_rxy1P4stAuLJ$>, thanks for your feedback on this.
Thus far I did not consider the fact that users might feed W2W a geo_em file that already contains LCZ labels, numbered from 50-61, and derived from the MODIS-CGLC-LCZ dataset.
This makes things even more confusing.
What I'd like to suggest:
* When launching W2W, it checks for the existence of LCZ labels 50..61
* if yes, relabel these pixels to 13 => continue the tool (removing urban, adding LCZs, adding parameters, ...)
* if no, continue the tool (removing urban, adding LCZs, adding parameters, ...)
This starts to become a bit of a circular behaviour, but at the moment I do not see another solution?
I briefly checked this with @andreazonato<https://urldefense.com/v3/__https://github.com/andreazonato__;!!D9dNQwwGXtA!SfYJ9mqClESjywhHvYKXq8q8HEOybxxNa10Lpy_YiYvG4Pcy3dj_mu3j-JbIuXgHSVfZ-efxEA8_rxy1PwSAnm5n$> as wel, and he also thinks this is fine.
So, I can try to add this, and then hopefully you can test again (with a WRF version that understand labels 30..41 and labels 50..61)?
M.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/matthiasdemuzere/w2w/pull/127*issuecomment-2003979333__;Iw!!D9dNQwwGXtA!SfYJ9mqClESjywhHvYKXq8q8HEOybxxNa10Lpy_YiYvG4Pcy3dj_mu3j-JbIuXgHSVfZ-efxEA8_rxy1P1ThzG_c$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABAA5Y6OXHB2ONLSMEWLWALYY3WXHAVCNFSM6AAAAABESJ2WD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBTHE3TSMZTGM__;!!D9dNQwwGXtA!SfYJ9mqClESjywhHvYKXq8q8HEOybxxNa10Lpy_YiYvG4Pcy3dj_mu3j-JbIuXgHSVfZ-efxEA8_rxy1P0mb9z_8$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I see. And is ISURBAN == True the same as LU_INDEX isin [50...61]? If your code does more or less what I suggested, then feel free to add it to this branch. Then I can have a look! Thanks! |
ISURBAN is an attribute of the netCDF file and it is a number (13) not a boolean.
So if you check which grid points meet ISURBAN, in the new files with CGLC-MODIS, it will be none.
I will add my changes to the branch.
On 18 Mar 2024, at 15:22, Matthias Demuzere ***@***.***> wrote:
I’ve done that already, it does something similar to what you suggest, except that those changes are implemented only where the code checks ISURBAN.
I see. And is ISURBAN == True the same as LU_INDEX isin [50...61]?
Note that I am not sure what ISURBAN does? Is it used elsewhere in WRF? The reason I am asking is that I don't think W2W changes this layer, after implementing the new LCZ-based extent?
If your code does more or less what I suggested, then feel free to add it to this branch. Then I can have a look! Thanks!
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/matthiasdemuzere/w2w/pull/127*issuecomment-2004054014__;Iw!!D9dNQwwGXtA!RTkum9YXmFWLue_g8e6pPytoF6s2jqDVPdIqSSBMAgY9clhqc6AXOnHejVJ-mqczdMGBUc7K0DH_D0PPamYBRYWK$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABAA5Y3KZG5K4HEJZ5LCCXDYY32CVAVCNFSM6AAAAABESJ2WD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBUGA2TIMBRGQ__;!!D9dNQwwGXtA!RTkum9YXmFWLue_g8e6pPytoF6s2jqDVPdIqSSBMAgY9clhqc6AXOnHejVJ-mqczdMGBUc7K0DH_D0PPalit7x7R$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@dargueso: hold your horses! I was just thinking about an alternative strategy ... see email for follow-up! |
w2w/w2w.py
Outdated
luf[dst_data.ISURBAN-1]=np.sum(luf.values[30:41,:],axis=0) | ||
luf[30:41,:]=0 | ||
|
||
|
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 wonder whether this should be formulated more generically? In the following sense:
For the built LCZ types, W2W considers by default LCZ labels 1 to 10, see here:
Line 80 in 84f7d80
parser.add_argument( |
This information is then stored in Info.BUILT_LCZ.
But, a user might also define eg. LCZ 15 (=LCZ E / Bare rock or Paved) as built-up, by providing this as an argument to the tool.
I am not sure if:
- anyhone has used this yet?
- the tool is actually fully functional when using this argument?
But, perhaps this option should be considered to some extend, by using:
- luse<=41 and luse<=61?
- and using :42 and :62, instead of :41 and :61?
Does this makes sense?
w2w/w2w.py
Outdated
urban_cat_list = [31, 32, 33, 34, 35, 36, 37, 38, 39, 40, urban_cat] | ||
else: | ||
urban_cat_list = [urban_cat] | ||
|
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.
see previous comment. Perhaps here the urban_cat_list can be defined depending on Info.LCZ_BUILT, that by default has [1 .. 10], but might also be [1 .. 11]?
urban_cat_list = LCZ_URBAN + [urban_cat] | ||
else: | ||
urban_cat_list = [urban_cat] | ||
|
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.
same comment here as above ...
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 we can clean that a little bit. Perhaps using Info.LCZ_BUILT and dst_data_orig.NUM_LAND_CAT. But not sure how to deal with those options you mention: LCZ 11 and LCZ 15
Hi @andreazonato, @dargueso, Here a quick summary of what we discussed at EGU24, including some tasks: The enhancement of W2W is still an intermediate solution, to make sure it still works with WRF versions >= v4.4.2. Until that time, users can use W2W in three different ways, depending on WRF version: Case A (WRF < v4.4.2)
Case B (WRF >= v4.4.2)
Required steps to finalize this development:
If something is not clear or missing, please report in this space! |
I'm already testing that the code works for the three cases and that the generated files work with their respective WRF versions.
It is obviously not the purpose of this repo or this discussion because it is a more general issue, but it affects w2w users when using the domains with urban areas removed. It is linked to this wrf-model/WRF#1878 and this #125 |
Hi guys, @matthiasdemuzere , thank you for summarizing. I do not think there are additional issues. Maybe we will still reply and help W2W users until the W2W is inside WRF ( I guess, end of the year, depending on the reviews on the paper) @dargueso Yes, this issue does not depend on w2w. I discussed it with Cenlin He, and we actually did not come to a solution actually, since there is no variable to stick to put a flag and decide whether to use LCZ or not. I will try to reach him again and see if we can find a possibility to solve it. Thank you for testing |
Thanks @dargueso. I hope all works? Also, @andreazonato mentioned the existence of a new matrix that was added, that contains, per pixel, the fraction of each LCZ label from the sub-grid scale information? I believe this variable should be added as a dummy, no? Should this be added only for WRF >= v4.4.2? @andreazonato ? |
@matthiasdemuzere I think there is no need to add additional variables. |
@andreazonato, just to make sure I 100% understand. At EGU you mentioned that, when users use “cglc_modis_lcz+default”, a matrix was added in geo_em, that contains the LCZ fractions for every pixel. Information that is then used within WRF to get the appropriate morphological parameters. But this functionality will only become available when your new WRF version is released? Until that time, this matrix variable is not needed / used? I just want to avoid that, when users use their own LCZ map from the .tif, in reality LCZ information from "cglc_modis_lcz+default" is used within the model? |
I think we're running in circles. I don't use LANDUSEF (the matrix @matthiasdemuzere was referring too all along and contains percentage of each land use category in each grid cell). That matrix is in the original geo_em, it is not added by @andreazonato' WRF version, but it may be altered. Now, if I ignore LANDUSEF in W2W as before, it will defined in a way that makes WRF resets all LCZs to some rural land use. This is because @andreazonato's code activated with use_wudapt_lcz looks into this variable, but previous WRF versions didn't . I found this in v4.5.2, but it probably happens some versions back too. My approach was thus to add info to LANDUSEF variable based on the category assigned by W2W to each pixel. Hence for LCZ pixels is 1 or 0, and is 1 only for one LCZ. There is no sub grid information in LANDUSEF. As far as I can tell, this prevents the issue below:
Is this clear? |
Sorry, my fault. I just find it all too confusing. But thanks for explaining further, I believe I understand now. I'll await that result before working further on this (tests, documentation, ...) |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Fixes to ingest geo_em files with LCZ labels from 31 to 40 or from 50 to 60.
for more information, see https://pre-commit.ci
Setting NUM_LAND_CAT to 21 in the NoUrb output, which has cities removed. Some formatting too.
…om orig_num_land_cat
Change order of defining new landuse
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
20 correspondends to 21 but w/o lakes
1dd459d
to
7801b3e
Compare
I allowed 20 categories as well. Tests are now passing, but we reduced test coverage meaning we are not testing:
I attached the test coverage here: you can generate that by running Would be great if someone could add a test for that - could be similar to the |
I get a lof of questions on the LCZ labels, and related errors because of using a specific WRF version.
See example here: #122
So I worked towards a more structural solution, where the user is required to provide the wrf version as an argument.
Based on this argument, the required parameters are set using this dict:
I tested the code locally, from a W2W perspective using the sample data.
As far as I can see it seems to work.
@jkittner and @andreazonato: could you have a look?
Also: