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

[Issue #333] Use just one request to download a date range #347

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from

Conversation

garlic-os
Copy link
Contributor

@garlic-os garlic-os commented Jun 16, 2022

Description

Implements the feature described in the third issue in Issue #333.

How Has This Been Tested?

Added tests (tests/test_issue_333.py) to ensure the processed data is the same with the new method of downloading the data and the old one.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have added an explanation of what your changes do and why you'd like us to include them.
  • I have written new tests for your core changes, as applicable.
  • I have successfully ran tests with your changes locally.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jlmaurer jlmaurer marked this pull request as draft June 23, 2022 17:13
jlmaurer and others added 2 commits June 23, 2022 12:26
flake8

Merge pull request dbekaert#343 from the-garlic-os/add-dev-dep-autopep8

Add dev dependency "autopep8"

Merge pull request dbekaert#346 from the-garlic-os/lint

flake8

Merge pull request dbekaert#343 from the-garlic-os/add-dev-dep-autopep8

Add dev dependency "autopep8"

Merge pull request dbekaert#349 from the-garlic-os/flake8

More flake8
Raise better error for missing subdatasets

Housekeeping

Housekeeping

Use "_" for unused parameter

Housekeeping

Removing used variable assignments, adding comments, adding type annotations, etc. as I go along

Type annotate main program arguments

A mostly complete model of the arguments passed to tropo_delay() as I gathered from analyzing the code. May not be correct for all cases. Still need to figure out what Arguments["Heights"][1] is.

Comment out unused function

Searching for the function name through the whole project with vscode search, I don't see it called anywhere, but I probably shouldn't outright delete it before asking about it.
@garlic-os garlic-os changed the title [Issue #333] Condense yr/mo/day/time into date [Issue #333] Use just one request to download a date range Jun 23, 2022
@garlic-os
Copy link
Contributor Author

This is the progress I've made so far. Crashes on an error happening in WeatherModel.self._uniform_in_z() involving three arrays not having the same dimensions. Successfully fetches a date range of ERA5 data in one request, but I still need to ensure the data is processed properly afterward.
Also, it looks like the subsequent processing can be parallelized for each date! 👀 Future PR, maybe?

- Removed or commented out unused names found by vulture
- Refactored some variable names to use snake_case
- Importing only from typing and not typing_extensions
- Replaced uses of `except BaseException` with just `except` along with a TODO comment to fill in the missing exception type
-
It looks like the otherwise unused `cpu_num` is meant to be passed to np.Pool().
A considerable number of function signatures changed as collateral damage, but this should allow RAiDER to calculate delays through ERA5 with only one CDS request for any range of dates.
Use super() for calling superclass constructor
@garlic-os
Copy link
Contributor Author

Working now in testing. It splits the downloaded date range into separate files for the rest of the program to digest as usual. From the testing I've done, the results seem to be the same as the were with multiple requests. But I need to write some tests so I can examine the change's validity beyond eyeballing the PDFs 😁

Following changes to _tropo_delay, it is no longer necessary as a processing layer to tropo_delay.
Fix dictionary formatting
Housekeeping

Housekeeping
Fix missing output files
@garlic-os
Copy link
Contributor Author

Added tests to ensure this change doesn't affect the outputted data.

@garlic-os garlic-os marked this pull request as ready for review July 28, 2022 06:18
@garlic-os
Copy link
Contributor Author

Fixed merge conflicts

Remove commented-out code
Data that the tests use to compare to live data.
This also fixes the failing enu2ecef and ecef2enu series of tests.
Rename datetime module to dt to remove ambiguity with datetime and datetime.datetime

Housekeeping

Housekeeping

Improve dict formatting
import datetime as dt
@garlic-os
Copy link
Contributor Author

Weird. It works on my machine. Is CircleCI running the tests from a different working directory?

@garlic-os
Copy link
Contributor Author

garlic-os commented Aug 4, 2022

Struggling to reproduce the error happening on CircleCI. Are the tests passing for anyone else? @jlmaurer

@garlic-os
Copy link
Contributor Author

While meeting with @jlmaurer this seems to have uncovered an issue with heights not being properly interpolated, which has also been causing issues elsewhere. Not able to properly test the changes in this issue until this other problem is fixed. Putting this PR on hold for now.

@jlmaurer
Copy link
Collaborator

@the-garlic-os I just updated the dev branch with the fix for ERA-5 API. Can you merge that with this and see if that solves the CI failures?

@garlic-os garlic-os closed this Aug 15, 2022
@garlic-os garlic-os deleted the improve-cds branch August 15, 2022 20:36
@garlic-os garlic-os restored the improve-cds branch August 15, 2022 20:36
@garlic-os
Copy link
Contributor Author

@jlmaurer Some errors in tools.RAiDER.dem that I think you should look at:

163,12,F,F821:undefined name 'writeDEM'
165,26,F,F821:undefined name 'X'

@garlic-os garlic-os reopened this Aug 15, 2022
@garlic-os
Copy link
Contributor Author

(Oops, I guess renaming a branch closes all its pull requests)

autopep8
Housekeeping
Remove download_only path - Per Jeremy this logic is no longer used.
stash dep changes
update environment deps
correct name of dem_stitcher conda package
Merge pull request dbekaert#339 from jlmaurer/fix_stitchdem
update dem.py with new dem-stitcher api
Merge branch 'dev' into update_deps
remove uneeded deps
Update ERA5 API
Remove unneeded deps
@jlmaurer
Copy link
Collaborator

@the-garlic-os yes those are bugs that I've fixed in #332 . Let's pause on this until that one gets merged.

Remove unused import

Update .gitignore

Remove unused argument

Ignore .pytest_cache/
No really for real this time
@garlic-os garlic-os closed this Jun 12, 2024
@garlic-os garlic-os deleted the improve-cds branch June 12, 2024 01:35
@garlic-os garlic-os restored the improve-cds branch August 12, 2024 19:04
@garlic-os garlic-os reopened this Aug 12, 2024
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