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

change datum default to MSL for coops_product_within_region #124

Conversation

FariborzDaneshvar-NOAA
Copy link

No description provided.

@SorooshMani-NOAA
Copy link
Contributor

@FariborzDaneshvar-NOAA the code in this repo is styled using black. Please install it and format the file you updated. Thank you!

@SorooshMani-NOAA
Copy link
Contributor

@pmav99 I'd like to ask for your opinion on this:

We're changing the default datum for COOPS from STND to MSL, since it makes more sense for it to be MSL. One way to fix the tests is to add the explicit datum request to be STND in the tests, another is to update the test VCRs and refs to reflect the new default. Which one do you prefer? I think the first one is better.

Another thing is do we want to warn the user of this change when they don't pass any value for datum or not?

@SorooshMani-NOAA
Copy link
Contributor

Actually we don't need to update the default in searvey ... let's just update the default in StormEvents for now, the we'll use the MSL default in the new COOPS API in searvey, that way we don't need to worry about warning the users of the change. @pmav99 @FariborzDaneshvar-NOAA what do you think?

@FariborzDaneshvar-NOAA
Copy link
Author

FariborzDaneshvar-NOAA commented Jan 23, 2024

Actually we don't need to update the default in searvey ... let's just update the default in StormEvents for now, the we'll use the MSL default in the new COOPS API in searvey, that way we don't need to worry about warning the users of the change. @pmav99 @FariborzDaneshvar-NOAA what do you think?

@SorooshMani-NOAA whichever that you think might raise less confusion. thanks

@SorooshMani-NOAA
Copy link
Contributor

See #99 (comment)

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