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

Some clean-up for key_metrics #122

Merged
merged 6 commits into from
Sep 4, 2024
Merged

Conversation

mnlevy1981
Copy link
Collaborator

@mnlevy1981 mnlevy1981 commented Aug 13, 2024

  1. Standardize the way dates are passed (glacier notebook pulls year out of full date string to match atmosphere)
  2. Add option for baseline comparison to atmosphere NMSE
  3. Update config.yml to use run 104 (keeping 092 as baseline)
  4. Clean up time series generation (allow empty list to specify "don't convert any files to time series for this component")

All Submissions:

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

1. Standardize the way dates are passed (glacier notebook pulls year out of
full date string to match atmosphere)
2. Add option for baseline comparison to atmosphere NMSE
3. Update config.yml to use run 104 (keeping 092 as baseline)
4. Clean up time series generation (allow empty list to specify "don't convert
any files to time series for this component")
@mnlevy1981
Copy link
Collaborator Author

There's not much change in the NMSE of surface pressure between 92 and 104:

439db59960de6dd183d7bc35c1f8c6ae294e6ceaa15ec20b14441c3a005a80d0

The surface mass balance is very different, likely due to (a) only averaging over 20 years instead of 40, and (b) only using 25 years for the spin-up instead of 60:

c133d0739aea764f4ac25e4f37c1a29ac106e5658bdc8a49b9fbe3dc431ed1aa

e6a1cf7998dad4b5061aae9d06572712a07fb4bc1d9e6c9f29e6791336fa1ff4

The x-axis of this plot could be extended -- there's 40 years of data from 092 but only the last 20 years are shown

664ca78d05d5241363c25efd1776f9d251cb72f32a38a8a2e92c7dcdcff9c718

@mnlevy1981
Copy link
Collaborator Author

I'm also tempted to update the notebooks to hide input cells (as noted in #60 (comment))

1. Only set base_last_year if a baseline case has been provided
2. Update time series plots to account for differences between climo_nyears and
base_climo_nyears
Also fixed a bug in the NMSE plotting
For key_metrics examples, the hide-input tucks the python code away behind a
drop-down menu. There is one cell in the glacier notebook that uses hide-cell
instead of hide-input because the output isn't important (printing a message
about the number of years used to compute climatology)
@mnlevy1981
Copy link
Collaborator Author

mnlevy1981 commented Aug 28, 2024

Still to-do:

  • Resolve max_years_per_timeseries_file discussion
  • Update 104 to include 100 years of output, for more apples-to-apples comparison

Updated the timeseries block of config.yml to include both case_name and
base_case_name; this uncovered a few issues in constructing the arguments for
the call to create_time_series()
@mnlevy1981
Copy link
Collaborator Author

Updated figures comparing 100 years of each run:

NMSE

fdb7a2c0abedc4ea96c9ed7808b768771b74b8d8cc81354f5a743a9c4376bfb7

SMB

bd9de61aff4798bc03bc0aef11bb46042e35a29991bc0ab785edcf7c8192bc44
b3d391f5abd5aa12e95535aa59bba6cc3c022b7f8cc0d6300f6ef1ad1f2fd0fb
767cc7e08d4e14b4dfaf8235a67fdaa633581b30874354d2ac2e28c00688b564

I really like how the 104 vs 92 SMB plot has a sign change just above the southern tip of the island, that seems pretty interesting. Anyway, this is ready for final review / merging :)

@mnlevy1981 mnlevy1981 marked this pull request as ready for review August 28, 2024 22:26
@mnlevy1981
Copy link
Collaborator Author

It's interesting that the black line in

time series

doesn't extend as far as the red and blue lines... going to try to fix that real quick

@mnlevy1981
Copy link
Collaborator Author

It's interesting that the black line in

time series

doesn't extend as far as the red and blue lines... going to try to fix that real quick

fixed in c0ab02a

109c424112e4a084600f18d6cab6c69029b07bb5ce79eeacd1689bccbb4662f9

@TeaganKing
Copy link
Collaborator

@mnlevy1981 is this ready for review?

@mnlevy1981
Copy link
Collaborator Author

Yup, I think this is ready to go. Latest output is in #122 (comment) (except for the time series plot, which is updated in #122 (comment))

@TeaganKing TeaganKing self-requested a review August 29, 2024 21:22
Copy link
Collaborator

@TeaganKing TeaganKing left a comment

Choose a reason for hiding this comment

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

Thanks for doing all of this clean up! The method for passing dates is looking much clearer, and the capability of adding an empty list is helpful. The plot is looking a lot better, too! I'm going to test this out locally as well.

Copy link
Collaborator

@TeaganKing TeaganKing left a comment

Choose a reason for hiding this comment

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

Looks good to me! Local testing worked smoothly. All tasks are completed despite the tasklist checker failing... I think this is because the last checkbox was checked off after the last commit was made.

@mnlevy1981 mnlevy1981 merged commit 17e745c into NCAR:main Sep 4, 2024
1 of 2 checks passed
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.

2 participants