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

Alignment of skill dimension naming #460

Closed
aaronspring opened this issue Sep 26, 2020 · 9 comments · Fixed by #478
Closed

Alignment of skill dimension naming #460

aaronspring opened this issue Sep 26, 2020 · 9 comments · Fixed by #478

Comments

@aaronspring
Copy link
Collaborator

aaronspring commented Sep 26, 2020

Handled in three different ways that can easily be aligned, but how?

  • hindcast.verify(reference=['historical','persistence']).skill # initialized, historical, persistence
  • PM allows both: historical and uninitialized pm.verify(reference=['historical','unintialized','persistence']).skill # ['initialized','historical','unintialized','persistence']
  • bootstrap returns skill dimension: init, uninit, pers

proposed solution:

  • hindcast: rename historical to uninitialized matching the dataset name
  • PM: disallow historical
  • rename bootstrap accordingly to initialized, uninitialized, persistence

are you OK with this? @bradyrx

@aaronspring
Copy link
Collaborator Author

so the general question is: should we have two names for the same thing? historical and uninitialized. I know there are different communities and usages. I prefer to have a common name and a clear documentation on this.

@aaronspring aaronspring added this to the climpred v2.1.1 milestone Sep 26, 2020
@aaronspring aaronspring mentioned this issue Sep 26, 2020
25 tasks
@aaronspring aaronspring changed the title Alignment of skill: inconsistency _datasets['uninitialized'] & repr uninitialized vs reference=['historical'] Alignment of skill dimension naming Sep 29, 2020
@bradyrx
Copy link
Collaborator

bradyrx commented Sep 29, 2020

Yes I agree @aaronspring . I noticed this and meant to bring it up. Well, I agree that we should have one name. The question is, does it make sense to have it 'historical' or 'uninitialized'? I think it makes more intuitive sense to be 'historical', meaning you could put in any single "historical" climate run and evaluate against that. However, that practice isn't really used outside of the S2D community. So I'd say for the community-sake, we'd do 'uninitialized'. Although it seems to be really Steve Yeager/CESM-DPLE that uses that terminology. And as a reviewer pointed out in my ocean acidification paper, 'uninitialized' doesn't make sense to them since the model is of course initialized.

So I actually would favor 'historical' for both, but we already use compute_uninitialized and so on. We can stick with 'uninitialized' and just continue to document it well.

@aaronspring
Copy link
Collaborator Author

happy with unintialized, initialized and persistence.
Hongmei and Tatiana also use uninitialized, me in my paper also.

And as a reviewer pointed out in my ocean acidification paper, 'uninitialized' doesn't make sense to them since the model is of course initialized.

You can say that your model was initialized maybe once but not when ensemble members where generated.

So I actually would favor 'historical' for both, but we already use compute_uninitialized and so on. We can stick with 'uninitialized' and just continue to document it well.

planning to deprecate compute_uninitialized #468

Great I will do that PR!

@bradyrx
Copy link
Collaborator

bradyrx commented Sep 29, 2020

Agreed on all of this! Yes I think when I first mention DPLE I say it's "re-initialized" many times and we'll call it "initialized" and that CESM-LE was initialized once and we'll call it "uninitialized".

@aaronspring
Copy link
Collaborator Author

Reinitialized is nice

@aaronspring
Copy link
Collaborator Author

probably we discussed this before. we have .add_observations() but technically this can also be a reconstruction. what about renaming to .add_verification() but leaving .add_observations() but with a deprec. future. warning? @bradyrx

@aaronspring aaronspring mentioned this issue Sep 30, 2020
8 tasks
@bradyrx bradyrx mentioned this issue Oct 6, 2020
14 tasks
@bradyrx
Copy link
Collaborator

bradyrx commented Oct 7, 2020

probably we discussed this before. we have .add_observations() but technically this can also be a reconstruction. what about renaming to .add_verification() but leaving .add_observations() but with a deprec. future. warning? @bradyrx

I think we switched it to this since this is the most common way people are verifying a hindcast -- against observations. People using this will understand that "reconstruction" is observations in the eyes of this system.

.add_verification() I don't think is as clean of a reference since verification is the process of scoring the hindcast against your observational product. Really you'd want .add_verification_data() which is too long. I would prefer .add_observations() here just to keep it simple.

@aaronspring
Copy link
Collaborator Author

Then we should also rename the repr. currently reads Verification data. @bradyrx preference? verification or observation?

@aaronspring aaronspring reopened this Oct 9, 2020
@bradyrx
Copy link
Collaborator

bradyrx commented Oct 9, 2020

I'll switch to "Observations". Addressed in #459 since it's minor.

@bradyrx bradyrx closed this as completed Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants