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

xarray for roti #120

Merged
merged 63 commits into from
Jul 28, 2022
Merged

xarray for roti #120

merged 63 commits into from
Jul 28, 2022

Conversation

JonathonMSmith
Copy link
Collaborator

Description

Addresses #92

Added xarray loading of CDF files to cdaweb.py using the cdflib cdf_to_xarray function.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Data has been loaded and compared to the CDAWEB viewer as well as the local unit tests.

Test Configuration

  • Operating system: OSX
  • Version number: [Python 3.9]
  • Conda 4.10 environment.

Checklist:

  • [sorta] 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
  • Update zenodo.json file for new code contributors

@JonathonMSmith JonathonMSmith changed the title Inst/roti xarray for roti May 20, 2022
@JonathonMSmith
Copy link
Collaborator Author

Forgot that this has been developed around the load_meta_proc branch from pysat. Happy to close this if it's inappropriate.

@jklenzing
Copy link
Member

I say we keep developing this here and rely on local tests for the merge into the other PR. We can flag that one as waiting for next pysat.

@@ -247,7 +499,7 @@ def list_remote_files(tag=None, inst_id=None, start=None, stop=None,
'1900' will be added for years >= two_digit_year_break
and '2000' will be added for years < two_digit_year_break.
(default=None)
delimiter : string or NoneType
delimiter : str or NoneType
Copy link
Member

Choose a reason for hiding this comment

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

Strings don't need NoneType as a default unless there's a really good reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I got rid of the NoneType default for all the parameters that don't need it. There were a couple with more involved responses to the None value that I left alone. But I'm not 100% sure they couldn't get by with using '' in place of None

Copy link
Member

Choose a reason for hiding this comment

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

If you have a list of them, we can go over them at the next meeting.

jklenzing and others added 2 commits May 26, 2022 09:57
Co-authored-by: Angeline Burrell <aburrell@users.noreply.github.com>
STY: reduce duplication for init and clean
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.

This looks like a good start. A few style suggestions, but I can load the roti data as an xarray object. Some of the variables listed on cdaweb at https://cdaweb.gsfc.nasa.gov/pub/software/cdawlib/0SKELTABLES/gps_roti15min_jpl_00000000_v01.skt are not present in the loaded data (jan 1, 2014). I only see rotimed and roticnt. I'm not sure if not every file has every value, or if this is an issue on the cdflib side.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md 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/methods/cdaweb.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/cdaweb.py Show resolved Hide resolved
else:
estr = ''.join(["'time' already present in file. Can't rename ",
epoch_name, " to 'time'. To load this file ",
"it may be necessary to set `decode_times=True`."])
Copy link
Member

Choose a reason for hiding this comment

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

This option isn't available here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed this

pysatNASA/instruments/methods/cdaweb.py Outdated Show resolved Hide resolved
@jklenzing
Copy link
Member

This works locally with the latest pysat release. Can't re-run here, needs a commit to trigger this. You could merge in the latest develop changes if needed, but I think it's already downstream of the bug fixes.

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.

Looks good! I say we merge this into the other branch and tidy up the ROTI instrument there.

@jklenzing jklenzing merged commit ec16851 into pysat:inst/roti Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants