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

Maven mgs #200

Merged
merged 39 commits into from
Oct 5, 2023
Merged

Maven mgs #200

merged 39 commits into from
Oct 5, 2023

Conversation

t-esman
Copy link
Collaborator

@t-esman t-esman commented Sep 5, 2023

Description

Addresses #(issue)

Adds MAVEN pysat compatibility (No specific issue number).

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

    import pysat
    from pysat.utils import registry
    registry.register_by_module(pysatNASA.instruments)

    mag = pysat.Instrument(platform='maven', name='mag')
    mag.download(dt.datetime(2020, 1, 1), dt.datetime(2020, 1, 31))
    mag.load(2020, 1, use_header = True)

Test Configuration:

  • Operating system: Ventura
  • Version number: Python 3.9.13
  • Any details about your local setup that are relevant

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

If this is a release PR, replace the first item of the above checklist with the release
checklist on the wiki: https://github.com/pysat/pysat/wiki/Checklist-for-Release

pysatNASA/instruments/__init__.py Outdated Show resolved Hide resolved
.zenodo.json Show resolved Hide resolved
@jklenzing
Copy link
Member

It looks like a new dataset for ISS FPMU was uploaded, which is breaking the tests here. Out of scope for this PR.

@jklenzing
Copy link
Member

It looks like a new dataset for ISS FPMU was uploaded, which is breaking the tests here. Out of scope for this PR.

Added Issue #201 to cover the ISS FPMU bug. This is an upstream issue, so ignore it for the time being in the tests.

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

This is looking good. Instruments load data and metadata as expected.

Some style suggestions and updates based on the latest version. Examples applied to the insitu instrument, but changes should be similar for the others as well.

Also, please update the documentation for supported instruments: https://github.com/pysat/pysatNASA/blob/maven_mgs/docs/supported_instruments.rst

Thanks for putting this together!

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

Pinging @JonathonMSmith to see if the new cdasws routines could help streamline the new instruments as well.

@jklenzing
Copy link
Member

Hmm. The insitu data is now having issues with loading. Verified this via loading locally. Getting an error via xarray:

ValueError: broadcasting cannot handle duplicate dimensions on a variable: ['time', 'compno_3', 'compno_3']

The issue is with the general cleaning routine. fix incoming...

@jklenzing jklenzing added the enhancement New feature or request label Sep 7, 2023
docs/supported_instruments.rst Outdated Show resolved Hide resolved
docs/supported_instruments.rst Outdated Show resolved Hide resolved
docs/supported_instruments.rst Outdated Show resolved Hide resolved
pysatNASA/instruments/maven_insitu.py Outdated Show resolved Hide resolved
pysatNASA/instruments/maven_mag.py Show resolved Hide resolved
pysatNASA/instruments/maven_sep.py Show resolved Hide resolved
pysatNASA/instruments/maven_sep.py Outdated Show resolved Hide resolved
pysatNASA/instruments/maven_mag.py Show resolved Hide resolved
STY

Co-authored-by: Jeff Klenzing <[email protected]>
pysatNASA/instruments/maven_sep.py Outdated Show resolved Hide resolved
'/cdfs/{year:04d}/{month:02d}')),
'fname': fname}
download_tags = {'': {'': basic_tag}}
download = functools.partial(cdw.download, supported_tags=download_tags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since none of these datasets have monthly files this is good, but should we use the cdasws interface for MAVEN.
I'm still not sure that the cdasws interface is ideal for all of our NASA instruments but I can put together some suggestions if we want to use it here.

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.

I'll put together a pull to fix the clean routines

pysatNASA/instruments/maven_insitu.py Outdated Show resolved Hide resolved
pysatNASA/instruments/maven_insitu.py Outdated Show resolved Hide resolved
Comment on lines 70 to 79
# Check for symmetric dims
# Indicates transformation matrix, xarray cannot broadcast
if self.pandas_format:
# True by default
unique_dims = True
else:
# Check for multiple dims
unique_dims = len(self[key].dims) == len(np.unique(self[key].dims))
# Skip over the coordinates when cleaning
if key not in coords:
if key not in coords and unique_dims:
Copy link
Member

Choose a reason for hiding this comment

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

Change to allow a list of variables to skip as an optional kwarg.

pysatNASA/instruments/maven_sep.py Outdated Show resolved Hide resolved
t-esman and others added 4 commits September 25, 2023 10:28
Removal of the sep tag ''. Change of insitu to insitu_kp. Resulting necessary STY changes for flake.
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.

Clean routine incoming via pull request. A few suggestions here, otherwise looking good and working as expected.

Noting that there are a few instances where the time index is NaN in the sep file.

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

One more change: The standard elsewhere has been to have a mission-level reference that gets added to each instrument (this happens in the standard init routine). Since insitu_kp is copying multiple instruments, leaving this blank and just using the mission level is probably fine here.

pysatNASA/instruments/methods/maven.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/maven.py Outdated Show resolved Hide resolved
pysatNASA/instruments/methods/maven.py Outdated Show resolved Hide resolved
t-esman and others added 2 commits October 3, 2023 09:25
BUG: MAVEN general clean routine fix
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.

Minor updates, looking good.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/supported_instruments.rst 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.

Looks good!

@jklenzing jklenzing merged commit 8945d9d into develop Oct 5, 2023
21 checks passed
@jklenzing jklenzing deleted the maven_mgs branch October 5, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants