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

GUVI #107

Closed
wants to merge 0 commits into from
Closed

GUVI #107

wants to merge 0 commits into from

Conversation

landsito
Copy link
Contributor

@landsito landsito commented Apr 6, 2022

Description

Addresses # (issue)

Adding support for TIMED-GUVI L1C disk data (v13) into pysatNASA.

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Simple local script indicated in comments:

import pysat
from datetime import datetime
guvi = pysat.Instrument(platform='timed', name='guvi')
guvi.download(start=datetime(2005, 6, 20), stop=datetime(2005, 6, 21))
guvi.load(yr=2005,doy=171)

Test Configuration

  • Operating system: [Os Type]
  • Version number: [Python 3.9]

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

Copy link
Collaborator

@rstoneback rstoneback left a comment

Choose a reason for hiding this comment

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

Thanks Luis! I have a round of comments, some on style, and some on the loaded data format in xarray.

pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/cdaweb.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/cdaweb.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
@rstoneback
Copy link
Collaborator

Friendly note that the formatting notes point to specific examples but are intended to apply throughout the file.

@landsito
Copy link
Contributor Author

landsito commented Apr 7, 2022

I have included time variables according to pysat standards. timeDayAur is left as a secondary index in case of future need to use it. Geographic coordinates, altitude and solar zenith angle were also added as coordinates into the output dataset. The pytest routines successfully passed from my side. Finally, I checked the pep8 standard on this script.

Copy link
Member

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

More comment, mostly about style. Looks like you got to at least some of the files between the times I started and ended the review, so some of these may be outdated.

pysatNASA/instruments/__init__.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/cdaweb.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/cdaweb.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
@aburrell
Copy link
Member

aburrell commented Apr 7, 2022

Also, an entry in the "supported_instruments" documentation file needs to be added.

@landsito
Copy link
Contributor Author

landsito commented Apr 7, 2022

I just added a few style considerations recommended before. Thanks.

@landsito
Copy link
Contributor Author

landsito commented Apr 7, 2022

Checked style code with flake8 and autopep8.

@landsito
Copy link
Contributor Author

landsito commented Apr 7, 2022

Added this instrument into 'supported instruments' section.

Copy link
Member

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

The doc tags were in the wrong order. Also, some style updates weren't made across the entire file.

docs/supported_instruments.rst Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
@aburrell
Copy link
Member

aburrell commented Apr 7, 2022

Looks like there's some bugs in the download and remote file listing functions. Feel free to poke me if you need help debugging or need the CI tests to run again.

Copy link
Member

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

Suggestions for your work in progress.

pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
pysatNASA/instruments/timed_guvi.py Outdated Show resolved Hide resolved
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.

Thanks @landsito! The tests are running locally for me. The CI server is hanging on some formatting issues. I've added some comments that should fix those (though they may different in the upstream branch).

You can check these locally by running

flake8 . --count --select=D,E,F,H,W --show-source --statistics

Comment on lines 131 to 133
tags_fmt = {tag: fname.format(res= "-2" if 'low' in tag.split('_')[1] else "",
mode=tag.split("_")[0].upper())
for tag in tags.keys()}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tags_fmt = {tag: fname.format(res= "-2" if 'low' in tag.split('_')[1] else "",
mode=tag.split("_")[0].upper())
for tag in tags.keys()}
tags_fmt = {tag: fname.format(res="-2" if 'low' in tag.split('_')[1] else "",
mode=tag.split("_")[0].upper())
for tag in tags.keys()}

Comment on lines 143 to 144
tags_url = {tag: url.format(mode='imaging') if 'img' in tag else
url.format(mode='spectrograph') for tag in tags.keys()}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tags_url = {tag: url.format(mode='imaging') if 'img' in tag else
url.format(mode='spectrograph') for tag in tags.keys()}
tags_url = {tag: url.format(mode='imaging') if 'img' in tag else
url.format(mode='spectrograph') for tag in tags.keys()}

Comment on lines 239 to 240
inners = {dim : xr.concat([inners[dim], jnners[dim]], dim=dim)
for dim in dims }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inners = {dim : xr.concat([inners[dim], jnners[dim]], dim=dim)
for dim in dims }
inners = {dim: xr.concat([inners[dim], jnners[dim]], dim=dim)
for dim in dims}

Comment on lines 270 to 272
data = data.drop_vars(["YEAR_GAIM_DAY", "DOY_GAIM_DAY",
"TIME_GAIM_DAY", "TIME_GAIM_NIGHT",
"YEAR_GAIM_NIGHT", "DOY_GAIM_NIGHT"])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = data.drop_vars(["YEAR_GAIM_DAY", "DOY_GAIM_DAY",
"TIME_GAIM_DAY", "TIME_GAIM_NIGHT",
"YEAR_GAIM_NIGHT", "DOY_GAIM_NIGHT"])
data = data.drop_vars(["YEAR_GAIM_DAY", "DOY_GAIM_DAY",
"TIME_GAIM_DAY", "TIME_GAIM_NIGHT",
"YEAR_GAIM_NIGHT", "DOY_GAIM_NIGHT"])

data = data.assign(time_day=xr.DataArray(day_dts, dims=('nAlongDay')),
time=night_dts)
if 'img' in tag:
dt_aur=xr.DataArray(aur_dts, dims=('nAlongDayAur'))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dt_aur=xr.DataArray(aur_dts, dims=('nAlongDayAur'))
dt_aur = xr.DataArray(aur_dts, dims=('nAlongDayAur'))

"nCrossDayAur": data.nCrossDayAur.data}
elif 'spect' in tag:
coords = {"nchan": ["121.6nm", "130.4nm", "135.6nm",
"LBHshort", "LBHlong","?"]}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"LBHshort", "LBHlong","?"]}
"LBHshort", "LBHlong", "?"]}


# Sort
data = data.sortby("time")

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

# Instrument test attributes

_test_dates = {'': {tag: dt.datetime(2005, 6, 28) for tag in tags.keys()}}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

data = data.swap_dims({"nAlongNight": "time"})

# Update time variables
# 'time_night' will be renamed as 'time' to follow pysat standard
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# 'time_night' will be renamed as 'time' to follow pysat standard
# 'time_night' will be renamed as 'time' to follow pysat standard

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.

None yet

4 participants