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

Issue #13299 Asyncronous download of specific parameters to CSV and N… #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mfegan
Copy link
Contributor

@mfegan mfegan commented Jul 24, 2018

…etCSF is inconsistent

  • Insure lat/lon, if available, are included in JSON format
  • Revert removal of stream name from external parameters in CSV and JSON formats

Will update release notes once the code changes are approved.

…etCSF is inconsistent

- Insure lat/lon, if available, are included in JSON format
- Remove stream name from external parameters in CSV and JSON formats
Copy link
Contributor

@tlfish3638 tlfish3638 left a comment

Choose a reason for hiding this comment

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

The code changes appear ok but you need to check on the failed nosetest

Copy link
Contributor

@rjelkins rjelkins left a comment

Choose a reason for hiding this comment

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

may need to split out the stream name removal and give users a chance to provide feedback on if that's what they want in the csv and json data interfaces.

@@ -137,6 +137,13 @@ def _create_csv(self, dataset, filehandle):
# filter the dataset
dataset = dataset.drop(drop.intersection(dataset))

# rename the parameter without the stream_name prefix (12544 AC1)
Copy link
Contributor

Choose a reason for hiding this comment

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

for csv's and json they don't have the extra metadata for each variables that netcdf has to identifies the actual stream from which the parameter came from. netcdf can get away with removing the stream name from the variable because it has additional metadata attributes that help identify the true source of the data.

removing the stream name introduces a change in the interface of the data which may break user developed m2m tools. before implementing this change for csv's and json we probably should get feedback from the user community if this is what they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Will pull the name changes and update the ticket to request input/direction from the user community.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this will also correct the test failure.

…etCSF is inconsistent

- Insure lat/lon, if available, are included in JSON format
- remove name change for externals in JSON and CSV packaging
Copy link
Contributor

@danmergens danmergens left a comment

Choose a reason for hiding this comment

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

please update RELEASE_NOTES.md

@@ -130,14 +130,18 @@ def _deployment_particles(self, ds, stream_key, parameters, external_includes):
if lat_data is not None and lon_data is not None:
data['lat'] = lat_data
data['lon'] = lon_data
params.extend(('lat', 'lon'))
# always include lat/lon
params.extend(('lat', 'lon'))
Copy link
Contributor

@danmergens danmergens Jul 29, 2018

Choose a reason for hiding this comment

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

Is it correct to include lat/lon when we do not have values for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lines 127 - 132 apply to gliders only and essentially rename GPS based lat/lon as simply lat or lon. Some non-glider data does have lat/lon, e.g. GI01SUMO_RID16_02_FLORTD000, which were not being reported. At this point, we are actually saying we want to include lat/lon; if they are not available, they are reported as missing in lines 161 - 163 and removed from the requested param list in 165.

I think it is more efficient to do this than to attempt to determine ahead of time if lat/lon are available.

Copy link
Contributor

@sgfoote sgfoote left a comment

Choose a reason for hiding this comment

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

In netcdf_generator.py, external parameters associated with directly requested parameters are added to the list of parameters to be included with the request (i.e. not filtered out - see "params_to_include.append(parameter.name)"). Then they are renamed if applicable. In csvresponse.py, you are simply renaming the streams after the filtering is already done.

In other words, you seem to perform the equivalent of the NetCDF code for ticket 12544, but leave out the equivalent of the NetCDF code for ticket 12886. I don't know whether or not 12886 should apply to CSV.

Copy link
Contributor

@danmergens danmergens left a comment

Choose a reason for hiding this comment

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

Sounds good. Let's get updates to the release note posted in this review and fix the title.

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.

5 participants