Repository navigation
Conversation
e5d17da to
9504a41
Compare
gavinevans
left a comment
gavinevans
left a comment
There was a problem hiding this comment.
Thanks @Kat-90 👍
A couple of minor comments.
|
|
||
| """ | ||
| if not self.quantile_forest_installed: | ||
| msg = "The quantile_forest package is not installed. No parquet files have been processed." |
There was a problem hiding this comment.
I think I might make this something like, including updating the package name:
| msg = "The quantile_forest package is not installed. No parquet files have been processed." | |
| msg = "The quantile-forest package is not installed. Skipping training of Quantile Regression Random Forests." |
gavinevans
left a comment
gavinevans
left a comment
There was a problem hiding this comment.
Thanks @Kat-90 👍
I've added one comment.
| "The quantile_forest package is not installed. " | ||
| "No parquet files have been processed." |
There was a problem hiding this comment.
This warning message needs to be updated to match the warning message raised in the plugin. Without this update, the GitHub Actions are failing: https://github.com/metoppv/improver/actions/runs/39250826986/job/118053905714?pr=2459
| - sphinx-book-theme | ||
| - sphinx-gallery | ||
| - threadpoolctl | ||
| - xarray |
There was a problem hiding this comment.
@gavinevans & @Kat-90 is there a reason we are not adding these component to the latest.yml environment? I can see some value in keeping environment_b.yml without to test how the code performs with missing packages, but having these in latest would help alert us to issues coming out of package upgrades; I believe that's what the environment is for.
| forecast_df[column] = pd.to_datetime( | ||
| forecast_df[column], unit="ns", utc=True | ||
| ) | ||
| forecast_df[column] = pd.to_datetime(forecast_df[column], utc=True) |
There was a problem hiding this comment.
I'm a bit lost. The elements we read in here are assigned units of seconds because we force them to be so in the FORECAST_SCHEMA. We then set the units to ns. The values are unchanged throughout, we just change the units.
Why not modify the schema to have the units you want?
Why do we want ns rather than the pandas>3 default of us?
If we just stuck with actual factual s we could get rid of the seconds_to_ns that is applied our forecast_periods above.
I'm sure there are reasons that are passing me by, so just checking.
There was a problem hiding this comment.
We're now consistently using seconds as the unit for the time coordinates.
…in train QRF code.
Standardise unit handling in train QRF code
| - pygam<0.11 | ||
| - pysteps | ||
| - python-stratify | ||
| - quantile-forest |
There was a problem hiding this comment.
From the GitHub Actions: https://github.com/metoppv/improver/actions/runs/41612397845/job/125837287693?pr=2459#step:9:742, it seems like we need to include pyarrow as a dependency in both of these environments.
Switch to using seconds as the units
Updates from microseconds to seconds
bayliffe
left a comment
bayliffe
left a comment
There was a problem hiding this comment.
Thanks.
I assume this code will now be incompatible with our operational tables without some conversion, is that right and should it be documented for anyone trying to reuse those tables in experiments?
Addresses #GitHubissuenum
As an IMPROVER developer I would like to add a warning to check whether quantile_forest is installed in our environment or not.
The PR also adds the quantile_forest package to environment_a for use with unit testing.
Testing:
CLA