-
Notifications
You must be signed in to change notification settings - Fork 33
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 domain_query option #280
Conversation
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 think the proposed config format (domain_query=False
) is kind of confusing. In a sense it's still a query, just based on automatic lat/lon box. To me this feels more like a "domain query" than the original using site metadata already present in the dataframe.
Also, the Giorgi regions actually are rectangles so that's fine. But I would be skeptical about suggesting people use the EPA region rectangles, which are pretty rough estimates.
I have a little project for assigning to precise EPA regions. But it uses GeoPandas which is a heavy dependency.
@zmoon, should we call it something else then? domain_metadata=True (default, use metadata from observation file to determine domain) Open to other options too? |
I feel it could be done just with
|
I like this method too. This may be easier to explain in the docs too. |
OK, I'll try to add that now. |
Co-authored-by: Zachary Moon <[email protected]>
If we end up adding optional regionmask, that would include geopandas as a requirement. |
Co-authored-by: Zachary Moon <[email protected]>
@blychs do you want to mention these new option in the docs YAML section? |
I don't understand the failed tests... I don't believe I changed any of that? |
Co-authored-by: Zachary Moon <[email protected]>
@zmoon One of the tests is failing because it does not find noaa https://www.ncdc.noaa.gov/crn/ . Is it just out or does that link require updates? |
We are linking to data that normally is stable, but is located at the Asheville office, so it's likely that the recent hurricane took down some of their servers. Let's see if it's back up and running tomorrow and go from there. We can re-run the checks then. |
It seems the NCEI/NCDC websites are still having issues: https://www.ncei.noaa.gov/node/6696 |
@zmoon do you approve this now, since you have looked at this closer than me? If so, I will merge without the checks passing as it might take NOAA awhile to fix the servers in Asheville. |
@blychs were you able to test that it seems to work? |
Co-authored-by: Zachary Moon <[email protected]>
Wait, something is wrong. Nothing breaks (I had tested it) but it seems to not be doing anything. I'll fix it right away. |
OK, it was in effect being skipped. I had tested it with the airnow_wrfchem example, nothing broke, and I was happy with it. |
Solves Issue #278 .
As suggested by @rschwant , adds optional keyword "domain_query".
To avoid breaking old code, sets the default value for "domain_query" as True.
EDIT: I forgot to mention: I have tested this with airnow_wrfchem.ipynb, adding False domain_query and giorgi and EPA regions.
Cheers
Pablo