-
Notifications
You must be signed in to change notification settings - Fork 18
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
Change diags.py
to only return dataframes
#77
Conversation
@@ -6,12 +6,6 @@ | |||
from pathlib import Path | |||
|
|||
_VALID_LVL_TYPES = ["pressure", "height"] | |||
_VALID_CONV_DIAG_TYPES = ["omf", "oma", "observation", "hofx"] |
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.
how does one request hofx? do they need to manually grab Observation
and Observation_Minus_Forecast_Adjusted
?
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.
@CoryMartin-NOAA an hofx column is created when the dataframe is developed for all three classes. For example, see lines 104 and 114.
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.
Just saw a minor typo. However, don't want to approve/merge this right now as won't that break the develop branch? This needs to be coordinated with changes to the examples/scripts I think.
@kevindougherty-noaa we can create a working 'tag' in GitHub for people to use if you want to 'break' develop temporarily though. |
* updated scripts to handle changes in diags; small bigfixes in diags and plot_diags * remove ncfile scripts * remove netcdf_diags.py
* add methods to list values in respective classes * updated to include unique index vals in object
@CoryMartin-NOAA @aerorahul changes to |
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.
Few comments but overall looks good.
pyGSI/diags.py
Outdated
|
||
def _select_conv(self, obsid=None, subtype=None, station_id=None, | ||
analysis_use=False): | ||
def _select_conv(self, obsid, subtype, station_id, analysis_use=False): |
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 now is just analysis_use with a default? Shouldn't it be all or nothing for this internal method?
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.
Also the documentation below needs to be updated as appropriate for if there are/aren't defaults.
pyGSI/diags.py
Outdated
# Separate into 3 dataframes; assimilated, rejected, and monitored | ||
# First step, separate use_flag=1 and useflag!=1 | ||
good_use_flag_indx = np.where(self.chan_info['use_flag'] == 1) | ||
bad_use_flag_indx = np.where(self.chan_info['use_flag'] != 1) |
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 suggest not saying this is 'bad' per se, but rather 'monitored' Maybe have used_indx
and monitor_indx
or something like that?
pyGSI/plot_diags.py
Outdated
|
||
elif metadata['Diag File Type'] == 'radiance': | ||
if metadata['Channels'] == 'All Channels': | ||
title = title + '\nAll Channels' | ||
save_file = save_file + 'All_Channels' | ||
save_file = save_file + '_All_Channels' |
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.
nit picking here, but below is channels
and here it is All_Channels
, this should probably be lowercase?
scripts/mp_plot_sat_diags.py
Outdated
@@ -18,20 +17,54 @@ def plotting(sat_config): | |||
diag_type = sat_config['radiance input']['data type'][0].lower() | |||
channel = sat_config['radiance input']['channel'] | |||
qcflag = sat_config['radiance input']['qc flag'] | |||
# analysis_use = sat_config['radiance input']['analysis use'][0] |
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.
can this line that is commented out be removed? or is it a mistake?
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.
It should be uncommented. The current sample yamls do not include analysis use
as an option and forgot to uncomment.
Discussions were had to change
diags.py
to only return dataframes. This reduced overall code, however multiple scripts will need to be changed after the merge of this PR. Addresses #75