ForecastModel.set_query_latlon provides arguments in the wrong order to query.lonlat_box. Longitude should precede latitude.

[grisaitis]

Describe the bug
Forecast.set_query_latlon provides arguments in the wrong order to query.lonlat_box. Longitude should precede latitude.

To Reproduce

from pvlib.forecast import GFS

model = GFS()
model.connect_to_catalog()
model.longitude = [10, 350]
model.latitude = [-80, 80]
model.set_query_latlon()
print(model.query.spatial_query)

returns

OrderedDict([('west', -80), ('east', 80), ('south', 10), ('north', 350)])

Expected behavior

OrderedDict([('west', 10), ('east', 350), ('south', -80), ('north', 80)])

and this becomes an issue with GET requests when get_data is called... &west=-80&east=350&south=-80&north=80

Versions:

• pvlib.__version__: 0.5.2
• pandas.__version__: 0.23.0
• python: Python 3.6.5 :: Anaconda, Inc.

[wholmgren]

Thanks @grisaitis. I agree that the ForecastModel.set_query_latlon method passes the wrong arguments to query.lonlat_box (despite the correct code comment above the method call!).

The rest of the ForecastModel class assumes point data. We could go ahead and fix this bug in set_query_latlon in case someone someday takes on the work of generalizing the algorithms to work with gridded data (or statistics derived from it). Or we could delete the capability of querying for a range of coordinates. Not sure what the best approach is. @cwhanse thoughts on fix vs. delete in the context of Solar Forecasting 2?

Also note that test_forecast.py has a test_bounding_box function that is commented out because ForecastModel.get_query_data does not support coordinate ranges. If I remember correctly, an early version of the ForecastModel did support ranges but we removed them for simplicity before it was released.

[cwhanse]

@wholmgren Let's fix, it looks to be a small change.

[grisaitis]

After hearing what you said, @wholmgren, I think it actually makes sense to remove, not fix, this mention of lonlat_box. It's an easy fix, but it doesn't implement any behavior. This use of lonlat_box, to me, only makes sense as part of a broader implementation of bounding-box support in ForecastModel.

Proposal: remove this reference to lonlat_box. Create a separate issue for implementing bbox support in ForecastModel. Part of that will be adding back support for lonlat_box, along with other necessary changes to ForecastModel, etc.

Or feel free to still accept my PR :)

[grisaitis]

Some additional remarks about bbox support and ForecastModel:

I tried implementing bounding box support myself but gave up and just wrote my own script using siphon to get the data I needed. I borrowed a lot from ForecastModel -- constructing the query, converting the NCSS dataset to a DataFrame, etc -- but it was clear to me that too many changes were needed with ForecastModel to make it all work:

• ForecastModel assumed one Location for the whole forecast.
• process_data uses that one location for any solar position stuff
• THREDDS rejects requests that provide a vertical level and variables that don't exist at that level. THis isn't a problem with single point queries -- you can request e.g. surface temperature and then wind speed at 100000 Pa. But with a bbox, TRHEDDS will complain that the vertical level doesn't apply to all variables requested. So, you either have to avoid that situation by only requesting variables with the same level range (or no range at all), or perform multiple THREDDS requests, one per level needed (e.g. 1. wind speed at 100000 Pa, 2. surface temperature).

Related note: I think ForecastModel should be broken up into a few collaborators. E.g., converting raw data to processed data could be in another module entirely; why does it need to know that the data came from a NetCDFSubset? Also, instead of subclassing, it would be great to be able to instantiate ForecastModel with (a) a siphon NCSSDataset object and (b) a dict of variable name mappings. This would enable users to use other THREDDS servers, e.g. the GFS historical archive at UCAR . Not sure if there's an issue open about this but it could be pretty useful for some people.

[wholmgren]

@grisaitis thanks for the detailed report.

I agree that ForecastModel should be broken up and simplified. I would like to retain the simplicity of obtaining and processing data for points, but otherwise I have no particular thoughts about what it should look like. Your proposal sounds reasonable.

[grisaitis]

Thanks, @wholmgren. I think the API is great, and in fact useful if we want to offer the option of overriding certain built-in behavior like connecting to the latest GFS model, for instance. I'll open a separate issue about bbox logic. Any refactoring of ForecastModel can be implemented as part of that.

How about I change my PR to remove lonlat_box? And how do you feel about renaming set_query_latlon to set_query_lonlat, and generally changing anywhere else in the code to always be longitude then latitude? Upside: consistency. Downside: API change (changing method name.)

[wholmgren]

I'd suggest we leave the method name alone. It takes no arguments so there is nothing to gain from an API consistency perspective.

Let's go ahead and make the simple fix as first proposed. After that we can deal with a larger refactor to clean up the API and support gridded data. As I mentioned on the linked PR, the PR will need to be pointed to pvlib/pvlib-python/master rather than grisaitis/pvlib-python/master.

I've considered migrating the forecast fetching code to a pvlib.iotools package (see #261 among others). I don't have the time to put serious thought into this at the moment, but something to keep in mind when you're experimenting with a new API.

[grisaitis]

@wholmgren fyi -- fixed the PR base branch.

Thanks for heads up about iotools.