-
Notifications
You must be signed in to change notification settings - Fork 37
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
STY: updates to export options #1117
Conversation
@@ -1725,7 +1725,8 @@ def inst_to_netcdf(inst, fname, base_instrument=None, epoch_name=None, | |||
# Remove any attributes with the names below. pysat is responsible | |||
# for including them in the file. | |||
pysat_items = ['Date_End', 'Date_Start', 'File', 'File_Date', | |||
'Generation_Date', 'Logical_File_ID'] | |||
'Generation_Date', 'Logical_File_ID', 'acknowledgements', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to bring this question into the pull request.
Do these items still need to be popped?
We're assigning all of them in the following lines, so maybe they can be left un-popped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried swapping these out. Not sure if this is a remnant from python 2.7. acknowledgements and references have to be popped for compatibility with the operational bits below.
@JonathonMSmith, I'd like to get this one into the RC if possible. What's the best path forward to address the issues above? |
Are you sure you want to pull this into the RC and re-request all reviews? |
This is needed for the operational data stuff for REACH |
I don't know the best path forward. I removed the lines where these variables are popped in my local version of the RC branch and all of the unit tests pass, but I don't know if that's enough. I don't see how any of these attributes could get through without being overwritten because the the assignments on lines 1735-1756 are not conditional, the appear to always occur. |
My thought is open an issue. We probably want to test this more with existing operational software before removing this code. |
I've added #1122 for the next version. We can make a decision then. The rest of this still needs a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do references and acknowledgments need to be dropped from the attribute dict unless export_pysat_info is true?
I can't actually find where this happens in the code, but acknowledgements and references are transferred to the meta header info while the other pysat properties are not. If I don't pop these, then the unit test will fail since they still exist. I think it's because these are set during the init function. If I try this on
|
Description
Updates the pysat export options for operational instruments. Better integration with jklenzing/ops_reach#15
Type of change
How Has This Been Tested?
via updated unit tests
Test Configuration:
Checklist:
develop
(notmain
) branchCHANGELOG.md
, summarizing the changesIf 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