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

Expand testing on to_netcdf4 #585

Open
rstoneback opened this issue Oct 19, 2020 · 10 comments · Fixed by #1091
Open

Expand testing on to_netcdf4 #585

rstoneback opened this issue Oct 19, 2020 · 10 comments · Fixed by #1091

Comments

@rstoneback
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
to_netcdf4 failed for DMSP, #567. The underlying issue had been present for some time but nobody on the team tried to make a netcdf4 file from Madrigal data.

Describe the solution you'd like
We should include a to_netcdf4 test in the generalized testing so that all instruments are known to work.

Describe alternatives you've considered
Leaving support for exporting data to netcdf4 untested across the pysat family is certain to leave bugs in the works.

Additional context
Add any other context or screenshots about the feature request here.

@jklenzing jklenzing added this to the 3.1.0 Release milestone Jan 29, 2021
@jklenzing
Copy link
Member

I suspect this requires sorting out #60 first. I ran into issues writing from xarray objects, which is why xarray has its own unit tests for this rather than just inheriting from pandas.

@jklenzing
Copy link
Member

Once #896 is sorted out, we can start testing this out.

@jklenzing
Copy link
Member

What should success for this test look like? Is it

  • load existing data
  • write a copy of this using pysat's netcdf interface
  • load the new data and compare to the old?

@aburrell
Copy link
Member

aburrell commented Jan 5, 2022

The existing tests already do that. A good place to start would be coveralls, to see what is and isn't covered by the current tests.

@rstoneback
Copy link
Collaborator Author

We currently test that we can load all kinds of public datasets, we don't test that we can write those datasets using to_netcdf. In essence, we need to review all the metadata that we load.

@jklenzing jklenzing self-assigned this Dec 5, 2022
@JonathonMSmith
Copy link
Collaborator

I may not be in the right place, but to_netcdf is not working for cnofs IVM either.
in:

import pysat
import datetime as dt
ivm = pysat.Instrument(platform='cnofs', name='ivm')
d1 = dt.datetime(2012,1,1)
ivm.load(date=d1)
ivm.to_netcdf4('./ivmdat.nc')

returns:

out: TypeError: object of type 'float' has no len()

This appears to happen on line 1440 of pysat.utils.io when pysat.utils.io.return_epoch_metadata trys to take len of the epoch units which happen to be nan in this case. It seems like this statment is here to test that the dict value is empty. Unless it's likely that this value will be [], maybe instead of len the function could use == ''.

@aburrell
Copy link
Member

There should probably be a type check on the meta data. Under all of this is the assumption that units are a string. Since we allow the types to be updated, we need to check that this is, indeed, the case.

@jklenzing
Copy link
Member

Unless it's likely that this value will be [], maybe instead of len the function could use == ''.

I think this is the right call. a list is not allowed in meta.

@jklenzing jklenzing linked a pull request Mar 28, 2023 that will close this issue
10 tasks
@jklenzing
Copy link
Member

Given the number of changes/testing we would need to make in the libraries to implement this, I propose we bump the milestone.

@aburrell aburrell moved this from To do to In progress in Unit Test upgrades -- 3.1.0 Apr 4, 2023
@jklenzing
Copy link
Member

Bumping the milestone since this requires extensive testing with downstream packages.

@aburrell aburrell moved this from In progress to To do in Unit Test upgrades -- 3.1.0 Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants