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

Tiegcm #113

Merged
merged 31 commits into from Sep 1, 2022
Merged

Tiegcm #113

merged 31 commits into from Sep 1, 2022

Conversation

rstoneback
Copy link
Collaborator

@rstoneback rstoneback commented May 10, 2022

Description

Addresses #109

Adds TIEGCM data from ICON hosted via CDAWeb. Downloads are not tested due to file size. pysatModels now requires pysatNASA.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. Please also list any relevant details for
your test configuration

  • Tested locally by downloading data for Jan 10-11, 2020

Test Configuration:

  • Operating system: MacOS
  • Version number: 3.9
  • Any details about your local setup that are relevant: pysatNASA is close to develop branch, not released version.

Checklist:

  • Make sure you are merging into the develop (not main) branch
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • Add a note to CHANGELOG.md, summarizing the changes

@rstoneback rstoneback linked an issue May 10, 2022 that may be closed by this pull request
setup.cfg Outdated Show resolved Hide resolved
pysatModels/models/ucar_tiegcm.py Show resolved Hide resolved
@jklenzing
Copy link
Member

Should the ICON files be a separate tag?

@rstoneback
Copy link
Collaborator Author

image

@rstoneback
Copy link
Collaborator Author

I don't have a UCAR file to test with but the ICON times are now loading correctly. The ICON UCAR file stores the time units and epoch as part of the variable metadata which xarray now parses for us.

@rstoneback
Copy link
Collaborator Author

rstoneback commented May 10, 2022

test_remote_file_list is failing for UCAR TIEGCM. I can get the test to pass if I include a fake filename but I think that will lead to downstream issues. I'm going to think about things but we may need a flag for disabling that test for certain tags.

@jklenzing
Copy link
Member

Is this because the zip file is technically a different name?

@aburrell aburrell changed the base branch from rc_0.1.0 to develop May 11, 2022 16:22
@aburrell aburrell added this to the 1.0.0 release milestone May 11, 2022
@rstoneback
Copy link
Collaborator Author

No, the CDAWeb TIEGCM data is fine. It is the UCAR tag of '' that is the issue. There are no remote files to build a list from. I have a warning but the pysat test checks for non-zero number of filenames returned. I was thinking last night we could mod the test slightly to look for the warning, and it not present, check for returned filenames, or something like that.

@rstoneback
Copy link
Collaborator Author

Tests pass using pysat/pysat#1007 merged with load_meta_proc.

@aburrell aburrell modified the milestones: 1.0.0 release, 0.2.0 May 20, 2022
@rstoneback rstoneback changed the base branch from develop to main May 20, 2022 19:53
@rstoneback rstoneback changed the base branch from main to develop May 20, 2022 19:53
# Conflicts:
#	pysatModels/models/ucar_tiegcm.py
aburrell
aburrell previously approved these changes Aug 3, 2022
Copy link
Member

@jklenzing jklenzing left a comment

Choose a reason for hiding this comment

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

There's an additional ref for the ICON version:

Maute, A. Thermosphere-Ionosphere-Electrodynamics General Circulation Model for the Ionospheric Connection Explorer: TIEGCM-ICON. Space Sci Rev 212, 523–551 (2017). https://doi.org/10.1007/s11214-017-0330-3

Specifically, the versions we are grabbing here are generated using Hough Mode Extensions generated by ICON data, which probably should be mentioned in the docstring somewhere. The team has also been uploading a second set not driven by the HMEs at: https://cdaweb.gsfc.nasa.gov/pub/data/icon/l4/tiegcm_nohme/

Not sure we want to add the second set just yet. Long term I'd like to add the unzip support to pysatNASA, which involves more changes here, but that's a future pull.

@rstoneback
Copy link
Collaborator Author

Long term I'm fine with decompression support being built in somewhere. That's where I got the code from 😸

@rstoneback
Copy link
Collaborator Author

I've been waiting for all the tests to pass before merging. When I started re-running tests I only did the documentation tests but it somehow triggered a bunch more.

Anyway.... tests that take 30 minutes for Ubuntu are stilling running on windows after 3+ hours.

@rstoneback
Copy link
Collaborator Author

GitHub Actions is installing the problematic xarray version for some reason. Perhaps it is causing the windows issues? Trying it out.

@jklenzing
Copy link
Member

Starting work on another branch to see if the windows issues are there too.

@jklenzing
Copy link
Member

jklenzing commented Aug 10, 2022

EDIT: see pysat info in following comments

Definitely GA-related. My other branch is similarly slow. It looks like they updated the windows runner since the last time this branch worked:

Old version (20220710.1): https://github.com/actions/runner-images/blob/win22/20220710.1/images/win/Windows2022-Readme.md

New version (20220731.1): https://github.com/actions/runner-images/blob/win22/20220731.1/images/win/Windows2022-Readme.md

Skimming the list, I don't see anything that jumps out as breaking the code.

@jklenzing
Copy link
Member

jklenzing commented Aug 10, 2022

OK, it is related to the new pysat version. On ubuntu, the working version from 23 days ago ran 111 passed, 140 skipped, 142 warnings in 536.63s (0:08:56). The latest version is 242 passed, 2 skipped, 887 warnings in 996.45s (0:16:36).

Ran a new branch with a pysat cap: https://github.com/pysat/pysatModels/actions/runs/2834175762

All tests completed in the shorter amount of time with the new runners and scipy. I recommend setting a pysat cap here to get these pushed in, then we start troubleshooting the 138 new tests that were added with the latest pysat release elsewhere. EDIT: proposed patches in #119 to skip the broken tests and windows but run them elsewhere so that some coverage is achieved.

@jklenzing
Copy link
Member

Specifically, the culprit is the tests for instrument_altitude_to_model_pressure. Turning these off fixes the issue. See #118

@rstoneback
Copy link
Collaborator Author

Thanks for finding the area with an issue.... bummer it is my code 🙈

Tiegcm patch -- broken windows tests
requirements.txt Outdated Show resolved Hide resolved
@aburrell aburrell dismissed their stale review August 11, 2022 21:15

Stale review

README.md Outdated Show resolved Hide resolved
@rstoneback rstoneback merged commit 3550c43 into develop Sep 1, 2022
@rstoneback rstoneback deleted the tiegcm branch September 1, 2022 23:16
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.

ENH: Add ICON TIE-GCM data
3 participants