Skip to content

Breaking: fix problems with ncep types#98

Merged
rafaqz merged 5 commits into
masterfrom
better_ncep
Jun 23, 2026
Merged

Breaking: fix problems with ncep types#98
rafaqz merged 5 commits into
masterfrom
better_ncep

Conversation

@rafaqz

@rafaqz rafaqz commented Jun 17, 2026

Copy link
Copy Markdown
Member

There are some problems with NCEP. The "SixHourly" prefix on types is misleading. They are all six hourly by default, but have daily and monthly options as well in some cases.

Now we have the types Surface, Pressure and SurfaceFlux.

The "gause" in SurfaceGause was a desicription of the drid not the data kind. Thats an implementation detail, the files have crs attached. The grouping described in the docs is surface fluxes.

We also had dataset revision as a keyword. I've moved all of this to the NCEP type and made it consistent:

NCEP{Dataset,Revision,Period}

Where period can be ommitted to mean the standard six hours: NCEP{Surface,2} but you can do NCEP{SurfaceFluxes,1,Month}

This is a breaking change because these types are already in the wild. @mrke

@rafaqz rafaqz requested a review from tiemvanderdeure June 19, 2026 02:11
@rafaqz

rafaqz commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@tiemvanderdeure if you have time to review this it would be really helpful. I was lax on the initial review and let in some slop. I think this is a much better structure but would be good to get a second opinion before making the breaking version.

@tiemvanderdeure tiemvanderdeure left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, just commented on docstring :)

Comment thread src/ncep/ncep.jl Outdated
Comment on lines +31 to +32
Data from the NCEP/NCAR Reanalysis 1 (1948–present) and NCEP/DOE Reanalysis 2
(1979–present) datasets.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently discontinued per the 9th of March 2026... Maybe we should mention this or just write 1948-2026?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just put 2026 instead of present

Comment thread src/ncep/ncep.jl Outdated
- `Period`: `SixHour` (native, default), `Day`, or `Month`.

`NCEP{Surface, 2}` is the native 6-hourly surface analysis from Reanalysis 2;
`NCEP{Pressure, 1, Day}` is the daily Reanalysis 1 pressure-level aggregate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is at pressure levels right? So it would be a 3D dataset with pressure level steps? "pressure levels" in plural would be more correct, I'm not sure if the struct should also be PressureLevels, just because pressure sounds more like air pressure at ground level.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, yes PressureLevels is more correct

Comment thread src/ncep/ncep.jl Outdated
Data from the NCEP/NCAR Reanalysis 1 (1948–present) and NCEP/DOE Reanalysis 2
(1979–present) datasets.

See: https://psl.noaa.gov/data/gridded/data.ncep.reanalysis.html

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaqz rafaqz merged commit d23512e into master Jun 23, 2026
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants