ArtesiaWater / hydropandas

Module for loading observation data into custom DataFrames
https://hydropandas.readthedocs.io
MIT License
52 stars 11 forks source link

fix for 74 fill station not shown when all data is filled #139

Closed OnnoEbbens closed 1 year ago

OnnoEbbens commented 1 year ago

Okay this is a bigger than I anticipated. I think this PR should also solve issues #90, #92 and #131. On top of that I would like to:

OnnoEbbens commented 1 year ago

I won't fix #131 because it takes to much time for now

OnnoEbbens commented 1 year ago

@martinvonk, Thanks for the wow implementation. I approved and merged your wow pull request and then merged these changes into this pull request. It seems like everything is working fine now.

Some (minor) changes I made to the wow implementation:

Finally I would like to ask:

function with try/except:

def get_wow_metadata(stn: str) -> dict:
    try:
        r = requests.get(f"{URL_WOW_KNMI}/{stn}")
        r.raise_for_status()
        metadata = r.json()
    except requests.HTTPError as ex:
        raise ex
    return metadata

function without try/except:

def get_wow_metadata(stn: str) -> dict:
    r = requests.get(f"{URL_WOW_KNMI}/{stn}")
    r.raise_for_status()
    metadata = r.json()
    return metadata
martinvonk commented 1 year ago

I also fixed #131. Can you have a look at my changes @OnnoEbbens ? Then you can merge it :)

Nice work :)

OnnoEbbens commented 1 year ago

Thanks Martin, there are still some tests failing. Can you fix these?

martinvonk commented 1 year ago

Thanks Martin, there are still some tests failing. Can you fix these?

Yes, looking into it. Problem is that there is some overlap between the meteostation id's and precipitation station id's.

OnnoEbbens commented 1 year ago

Two things before merging:

  1. The error in the notebook occurs because some stations inside the json file cannot be accessed through the API nor without the API. I think these are the stations that are available in the json file but not available on this page -> https://daggegevens.knmi.nl/klimatologie/daggegevens . I would suggest to remove these from the json file.
  2. An empty file test.py was added to hydropandas/data, I think this file can be removed.
martinvonk commented 1 year ago

Two things before merging:

  1. The error in the notebook occurs because some stations inside the json file cannot be accessed through the API nor without the API. I think these are the stations that are available in the json file but not available on this page -> https://daggegevens.knmi.nl/klimatologie/daggegevens . I would suggest to remove these from the json file.
  2. An empty file test.py was added to hydropandas/data, I think this file can be removed.

fixed 2

on 1: There could be three things that go wrong;

martinvonk commented 1 year ago

new pandas version 2.1.0 seems to be the issue :)

OnnoEbbens commented 1 year ago

I tried to solve the issue with the new pandas version but it is hard to find what goes wrong. For now I will just merge this