-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add sea ice production/melting analysis tasks #907
Add sea ice production/melting analysis tasks #907
Conversation
FYI @xylar the obs data is now getting remapped, and the error I'm seeing is at the stage of plotting the remapped obs data, so I think we're almost there (thanks for the help today!). |
Hmm, I tried my suggestions above but the obs came out blank so there's debugging still to do: |
I ran this on a real run, and tweaked some of the plotting config options as a result: I'll talk briefly about this at tomorrow's check-in, but I think this is pretty much ready for review. |
Closes #216 |
@darincomeau, this needs to go in the description, not a comment. I'll take care of it. |
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.
This looks awesome and I'm excited to have it!
My one suggestion would be to go ahead and put everything in the same gallery, rather than having one with obs and one without. It's a little clunky either way since the thumbnails are different sizes but it makes comparing between seasons and annual a little bit easier:
I had mentioned that the filenames for the remapped observations looked weird to me. That's nothing you did -- MPAS-Analysis just gives remapped obs weird filenames, I guess. They contain both the original and the remapped grid resolutions, which was a surprise to me. Anyway, nothing we need to do anything about here.
@akturner and @milenaveneziani, please have a look when you can. It would be great to get this in! |
@xylar good suggestion, I've implemented it. I've also made the observations name change to AnIceFlux. Output now: One other thing I meant to solicit opinion on is whether this belongs in |
I think this should be on regardless. I think WC would be interested in it, not just cryosphere. |
This looks great! |
This reverts commit a884ed4.
Thanks @milenaveneziani ! I've reverted that change per your suggestion, and reran my test analysis linked above to reflect that. |
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.
Thanks Darin! this is good to go from my side.
@akturner, just a reminder that we're waiting on your review of this one. |
I'm taking @akturner off this for now. Seems like 2 reviews is probably enough. |
@darincomeau, thanks for the hard work on this! |
@darincomeau, one thing I forgot is that it would be great to have 4 new files added to the docs. You can see this one as an example. They're kind of formulaic but not so much so that I feel confident in writing yours. |
Obviously, since I merged this, it'll need to be its own PR. |
@xylar - sure, I'll put together those docs and open a PR. Also, this PR's functionality is broken on I tracked it down to the time gap between when I created this branch off develop and when #913 was merged a day later. I suppose I should have rebased, which would have caught this. Anyways, happy to leave the fix in #917, or if it makes sense to do in the doc PR, can do that as well. |
@darincomeau, I saw this too late but I think it's fine that the fix goes in in @cbegeman's PR even though it's unrelated. And sorry for missing that in my testing, too. |
Adds analysis task for climatology maps of sea ice total production and freezing, with comparisons to observations based on Haumann et al 2016. Note observations are only available for annual climatology, Southern Hemisphere.
closes #216