-
Notifications
You must be signed in to change notification settings - Fork 38
Sorcha/dev/571 #957
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
Sorcha/dev/571 #957
Conversation
@@ -0,0 +1,363 @@ | |||
## EXAMPLE USAGE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comment: you can directly make it an executable script this way:
https://github.com/ecmwf/WeatherGenerator/blob/develop/packages/evaluate/src/weathergen/evaluate/run_evaluation.py#L1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add this as a sepearte issue to be sorted after merging this one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is small enough and it is part of the basic ergonomics:
#!/usr/bin/env -S uv run
# /// script
# dependencies = [
# "weathergen-evaluate",
# "weathergen-common",
# ]
# [tool.uv.sources]
# weathergen-evaluate = { path = "../../../../../packages/evaluate" }
# ///
The script also needs to be executable: chmod +x zarr_nc.py
Let me know if you have any issues
Also: this script will eventually become the export entry point. How about calling it export_evaluation.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ended up calling it export_inference.py
, it only worked if I took logging outside of main though so might have to discuss this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sorcha,
Thanks for the script! I tested it on ATOS and it seems to work for me as well. I added a couple of comments. Happy to discuss them by chat if you want
Thank you for testing! Will push the edits later today :) |
Hi Sorcha, is this ready for review or still needs editing? |
Hi Ilaria, I've just moved the filepaths so it doesn't conflict with #693 , I will quickly check I wanted to see what you did with your quaver preprocessing before the review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enssow I have not reviewed the logic but high level comments, happy to talk about it
from weathergen.common.io import ZarrIO | ||
|
||
_logger = logging.getLogger(__name__) | ||
_logger.setLevel(logging.INFO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move all the logger logic to main. No side effects at imports
@@ -0,0 +1,363 @@ | |||
## EXAMPLE USAGE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is small enough and it is part of the basic ergonomics:
#!/usr/bin/env -S uv run
# /// script
# dependencies = [
# "weathergen-evaluate",
# "weathergen-common",
# ]
# [tool.uv.sources]
# weathergen-evaluate = { path = "../../../../../packages/evaluate" }
# ///
The script also needs to be executable: chmod +x zarr_nc.py
Let me know if you have any issues
Also: this script will eventually become the export entry point. How about calling it export_evaluation.py
# check config loaded correctly | ||
assert config["variables"]["q"] is not None | ||
|
||
FSTEP_HOURS = np.timedelta64(6, "h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a future PR: this should be retrieved from the run config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there's some care required with len_hrs
and the potentially temporal resolution of datasets (i.e. substeps in a window).
Description
Can run directly from the terminal:
uv run export --run-id uwbvdtge --stream ERA5 --output-dir /p/home/jusers/owens1/jureca/WeatherGen/test_output1 --format netcdf --type prediction target -- fsteps 2 1 --samples 1
or use the file directly with:
uv run ./packages/evaluate/src/weathergen/evaluate/export_inference.py --run-id uwbvdtge --stream ERA5 --output-dir /p/home/jusers/owens1/jureca/WeatherGen/test_output1 --format netcdf --type prediction target
export_inference.py
Issue Number
Closes #571
Checklist before asking for review
./scripts/actions.sh lint
./scripts/actions.sh unit-test
./scripts/actions.sh integration-test
launch-slurm.py --time 60