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

Satellite plotting fix, OMPS pairing fix, and some additional time interval looping #284

Merged
merged 11 commits into from
Oct 7, 2024

Conversation

mbruckner-work
Copy link
Collaborator

Fixes to satplots from @blychs.
OMPS-NM level 3 ozone pairing fix necessary for calculating daily mean from model's xarray dataset.
Enables time looped processing for RAQMS model, MOPITT CO obs. Small syntax change to allow use of time looping and OMPS-NM level 2 from Suomi NPP, NOAA-20, and future OMPS-NM instruments.

@blychs
Copy link
Collaborator

blychs commented Oct 4, 2024

The code added by @mbruckner-work looks fine to me. Since this PR includes some of my own commits, I guess we want an independent reviewer though?
Could @rrbuchholz or @dwfncar do it?

Copy link
Collaborator

@dwfncar dwfncar left a comment

Choose a reason for hiding this comment

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

All looks good.

@rschwant
Copy link
Collaborator

rschwant commented Oct 7, 2024

@mbruckner-work and @blychs, there is a small conflict now. Will Meng's method work here for you all too?

@blychs
Copy link
Collaborator

blychs commented Oct 7, 2024

@rschwant It should work (and probably be a better solution than ours).

@rschwant
Copy link
Collaborator

rschwant commented Oct 7, 2024

I'm testing that Meng's update still works with the aircraft code now. Do you all want to test that it works for your satellite options? Or if you are pretty confident it will work. I can choose Meng's option. We can merge it in. And everyone can retest everything this afternoon.

@blychs
Copy link
Collaborator

blychs commented Oct 7, 2024

I merged Meng's version, my code works fine and I don't think this can break Maggie's, but do you want to test it, just in case, @mbruckner-work ?

@mbruckner-work
Copy link
Collaborator Author

My test cases worked fine.

@blychs
Copy link
Collaborator

blychs commented Oct 7, 2024

Then I think it could be merged, @rschwant , unless we need another review.

@rschwant
Copy link
Collaborator

rschwant commented Oct 7, 2024

I'm worried that both of your all's fix for the filter_criteria that Quazi set up breaks the altitude feature Quazi added @quaz115. I'm looking into it.

@rschwant
Copy link
Collaborator

rschwant commented Oct 7, 2024

But I need to better understand Quazi's code to suggest a proper fix. I'm messaging him. Will fix soon.

@blychs
Copy link
Collaborator

blychs commented Oct 7, 2024

That's surprising. Is it breaking in the same line? Because we would expect both our previous solution and Meng's solution to not affect that, since the

if 'altitude' in ds_or_df:

should work worth for df and ds.
We could modify it to

if 'altitude' in ds_or_df.keys():

or, worst case scenario, for a dirty solution

if isinstance(pairdf, pd.DataFrame) and 'altitude' in pairdf:

@rschwant
Copy link
Collaborator

rschwant commented Oct 7, 2024

Okay it's fine. I'm having trouble calling the logic that Quazi set up as I don't fully understand the yaml options, but we update this later. The example runs well, so this can be pulled in.

@rschwant rschwant merged commit b243d09 into develop Oct 7, 2024
4 checks passed
@blychs blychs deleted the hotfix/satplots_and_aircraft branch October 7, 2024 22:47
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