Added treatment of USGS LU classes

[lpilz]

This PR adds the capability of using 24-category USGS class data to W2W.

I don't have any tests for it yet, as I wanted to ask whether you could create a test dataset in line with the other ones for me.

[jkittner]

Hey 👋
thanks for your PR - while I am not the right person to review the WRF details, I may be able to point you at the testing/5by5_20cat.nc. You can maybe use this as a template to add your additional classes and write some tests specific to the added feature. Right now it seems that the NotImplementedError is the cause for at least 4 failing tests, you may want to have a look at this first and check the conditions.
@matthiasdemuzere or @dargueso will have a look at the WRF-implementation details in a bit

thank you!

[lpilz]

Hey, thanks for the quick response - I fixed the broken tests. Just wanted to ask whether you generated the test-files using WPS or manually. And if you use WPS, could you provide me with a namelist? :)

[dargueso]

Hi @lpilz, Thanks for the PR. I'm having a look at your changes. The part where the codes discern which input data is used through the num_land_cat can be improved. This is is not your fault, the original code was probably too rigid and specific. I think we can use attributes in the geo_em files such as ISURBAN, ISLAKE or ISWATER, which define number for categories we use. Could you please send me your geo_em files so I can test with USGS too. That would also be useful to test why the code stalls for domain 1.

If we cannot use these attributes for whatever reason, I think we should better look into MMINLU to define the input data and get rid of all the hard-coded numbers for categories.

[lpilz]

Hi @dargueso, thanks for your response. I agree that the discernment is rather crude on my part and can definitely be improved. If you want, I can take a look at adding this to the PR. Please find my geo_em files attached. However, I'm not using the default USGS data, I'm using CORINE recategorized to USGS (https://zenodo.org/record/4432128).

geo_em.d01.nc.gz
geo_em.d02.nc.gz
geo_em.d03.nc.gz

[dargueso]

if hasattr(dst_data, 'ISLAKE')  and  0 < dst_data.ISLAKE < orig_num_land_cat

[dargueso]

Hi @dargueso, thanks for your response. I agree that the discernment is rather crude on my part and can definitely be improved. If you want, I can take a look at adding this to the PR. Please find my geo_em files attached. However, I'm not using the default USGS data, I'm using CORINE recategorized to USGS (https://zenodo.org/record/4432128).

geo_em.d01.nc.gz geo_em.d02.nc.gz geo_em.d03.nc.gz

I will look at the stalling issue in the next couple of days

[jkittner]

Looks good to me!
Only one kinda pedantic thing with inline if/else (and is not None) checks and proper use of booleans; but if you're fine @matthiasdemuzere we can leave it as is.

The only thing we're missing is a test, but so far no one provided a file, so since all the other functionality works (tests passing) I'm fine without a test for now.

Once this is merged and also the xarray pinning, we can make a new release 0.5.0 - what do you think?

[jkittner]

Suggested change

	is_lake = (
	dst_data.ISLAKE
	if hasattr(dst_data, 'ISLAKE') and 0 < dst_data.ISLAKE < orig_num_land_cat
	else None
	)
	if hasattr(dst_data, 'ISLAKE') and 0 < dst_data.ISLAKE < orig_num_land_cat:
	is_lake = True
	else:
	is_lake = False

I am not a huge fan of inline if/else - i'd write it like that, but I'm also fine with that.

What I'm wondering though is why else None and not False? I'd make is_lake a proper boolean and not nullable

[matthiasdemuzere]

Thanks for the review, for me it is fine to change the if/else structure.

Perhaps @lpilz or @dargueso can check the None or False statement?

[lpilz]

Hi, thanks for the review. I think the problem is the variable naming and I'll change that. Deriving the name from WRF's ISLAKE, is_lake is not a boolean, rather it's an integer, which is why I None'd it when it isn't defined. I use the None to selectively add some assignments e.g. in L472. I'll probably change the naming to something like lake_cat.

[matthiasdemuzere]

The only thing we're missing is a test, but so far no one provided a file, so since all the other functionality works (tests passing) I'm fine without a test for now.

Once this is merged and also the xarray pinning, we can make a new release 0.5.0 - what do you think?

Perhaps I was rushing this a bit. It would indeed be better to add a proper test, that checks the things specific to using usgs information. @lpilz, are you willing to provide a test file and test?

Also, we should probably update the README? To clarify that also USGS input can be used?
Or perhaps this is not needed, since in the README we do not explicitly refer to the MODIS input data? @lpilz: May I ask you to check this as well?

Once all is in place we can indeed make a new release 0.5.0!

[lpilz]

Hi @matthiasdemuzere, I agree that a test is necessary - that's why I kept the PR in Draft mode. To keep my test (and file) in line with the tests you're doing, I was wondering how you generated your test files. Do you do that manually or do you have some namelist.wps lying around?

[matthiasdemuzere]

@lpilz for test files we typically use existing geo_em files, are an area clipped from this. So perhaps you can use you own file to write a test for? Would that work?

[lpilz]

Hi @matthiasdemuzere, sorry for the delay. I'm currently very busy, so I will probably only make it middle/end of December. Hope this PR isn't too pressing....

[matthiasdemuzere]

Hi @matthiasdemuzere, sorry for the delay. I'm currently very busy, so I will probably only make it middle/end of December. Hope this PR isn't too pressing....

No worries!

[lpilz]

Hi there, hope you all had happy holidays! I finished implementing the tests for the wrf_remove_urban function, I hope this is to your liking. I also figured out the TODO which was in the test. The additional point being changed results from the fact that W2W removes the urban LANDUSEF from all points where it's non-zero and not only from those where it's the dominant category. In the 5by5 case, there are 3 points with dominant urban type and 4 points with non-zero urban type.

[matthiasdemuzere]

Hi @lpilz,

Thanks for your work on this, and your contribution to the W2W tool. All is looking good.
Enjoy your holidays too!

M.