Skip to content
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 module to read/interpret NCEI storm database #1962

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

gerritholl
Copy link

@gerritholl gerritholl commented Jul 9, 2021

Description Of Changes

Add a module with functionality to read storms from the NCEI storm database for a requested period in time. The functions rely on pint-pandas to add unit information.

Checklist

  • Closes #xxxx
  • Tests added
  • Fully documented

Start work on a module for reading and interpreting the NCEI (formerly
NCDC) storm database.  Work in progress.
Add a module with various functions to read and interpret events from
the NCEI storms database at https://www.ncdc.noaa.gov/stormevents/
@CLAassistant
Copy link

CLAassistant commented Jul 9, 2021

CLA assistant check
All committers have signed the CLA.

When reading NCEI storm data, don't fail on bad or old data, in
particular for timezones this is a concern.  Add test cases for all
timezones found in the NCEI storm data base, including typos and
erroneaus data.  Adapt the implementation of timezone interpretation to
use the 3-letter code (always available) rather than the 1-digit hour
offset (not always available).
Improve the documentation on reading storms from the NCEI database.
Improve the unit tests to cover additional cases.
@gerritholl gerritholl marked this pull request as ready for review July 9, 2021 15:57
@gerritholl
Copy link
Author

I've added API documentation, but when I make html it's not showing up in html/api/generated/metpy.io.html. What do I need to do to make the new module appear there? Should I import it from metpy.io.__init__.py? I'm not used to needing to do that in other packages, so maybe there's something particular in MetPy documentation building that I don't understand yet.

Clarify some of the documentation in the new storm reading module.
In the storm db module, replace double quotes by single quotes in most
cases, except in docstrings.
@dcamron
Copy link
Member

dcamron commented Jul 9, 2021

Thanks for this contribution! I've yet to do a review, but am glad to see this coming in.

To see your new docstrings added to the generated docs, you can import and use our Exporter class and its @Exporter.export decorator from package_tools.py; you can see usage of this in the other modules. This will add any functions you decorate to storms.__all__ and make them "public" as far as MetPy's API and the docs are concerned. Then, yes, you will need to extend the top-level io(the name we consider public) by importing your module within io.__init__ and extending io.__all__ as you see done for the other modules there. Then you should be able to import your code directly from the public io space, e.g. from metpy.io import get_noaa_storms_for_period and your docs should appear!

For the new NCEI storms DB reading functions, add the public functions
to the ``@exporter.export`` decorator and expose them in the parent
``io`` package.
Varous small docstring and import order fixes to satisfy flake8 with all
its varieties.
The new storms reading database needs fsspec.  Add this package to the
requirements.
@gerritholl
Copy link
Author

I don't know how to resolve the issue raised by the Codacy Static Code Analysis — it seems to arise from the magic concerning __all__ in src/metpy/io/__init__.py.

Resolving merge conflict in ci/requirements.txt.
@dopplershift dopplershift added this to the 1.2.0 milestone Aug 4, 2021
For the github-ci tests, add the missing dependency on pint-pandas.
@gerritholl gerritholl requested a review from a team as a code owner October 15, 2021 13:22
flake8 was complaining about typos in messages I copied verbatim from
the NOAA storms database.  Fix their spelling so that flake8 will be
happy.
@gerritholl
Copy link
Author

@dcamron @dopplershift My PR has a dependency on pint-pandas, a package which I think should fit well with the MetPy philosophy of associating units with numbers. However, there appears to be no package in conda-forge, leading to failing tests. Is pint-pandas an acceptable dependency or should I rewrite my PR to not use pint-pandas (and therefore not associate units with numbers)?

@dopplershift dopplershift modified the milestones: 1.2.0, 1.3.0 Nov 29, 2021
@dopplershift dopplershift modified the milestones: 1.3.0, May 2022 Mar 31, 2022
@dopplershift dopplershift modified the milestones: May 2022, July 2022 May 16, 2022
@gerritholl
Copy link
Author

I'm not sure when the tests were run, but looking at them to see the error I get under "install dependencies":

Error: The log was not found. It may have been deleted based on retention settings.

Are the tests only Windows and MacOs? No Linux?

@dopplershift
Copy link
Member

@gerritholl My impression of pint-pandas it that it's not particularly mature (@jthielen may have a more updated opinion), so I'm very reticent to depend on it at this time. Our (admittedly sub-optimal) solution to this point has been to add a .units attribute that contains a dictionary mapping column names to a unit string so that they're available for use later (by e.g. pandas_dataframe_to_unit_array).

If you can resolve the conflict in ci/requirements.txt (also please add a version to fsspec for whatever is current) and add fsspec to setup.cfg, that will trigger tests to run again.

@dopplershift dopplershift added Type: Feature New functionality Area: IO Pertains to reading data labels May 24, 2022
@dopplershift dopplershift removed this from the October 2022 milestone Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: IO Pertains to reading data Type: Feature New functionality
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

4 participants