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

Removed errorOnMissing and endYear = end config options #991

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Feb 29, 2024

This merge removes the config option errorOnMissing from the [input] section. In practice, we always want an error on missing data, rather than simply providing a warning using available data.

This merge also drops support for the end value for endYear. For developers, end has been more difficult to support than it is worth. For users, it isn't a great idea to have the same analysis run produce different results depending on how far along a simulation is. In practice, we always want to run fresh analysis when we want a longer time series.

We always want an error on missing data
@xylar xylar added the clean up label Feb 29, 2024
@xylar xylar requested a review from cbegeman February 29, 2024 08:50
@xylar xylar self-assigned this Feb 29, 2024
@xylar
Copy link
Collaborator Author

xylar commented Feb 29, 2024

@cbegeman and @milenaveneziani, please let me know if you agree that these are options we no longer want to support.

I was trying to fix endYear = end and decided I would rather just remove support.

Results are unpredictable and this option causes more trouble
for development than it is worth.
@xylar xylar force-pushed the remove-end-year-end branch from 78686de to 00a3101 Compare February 29, 2024 09:34
@xylar xylar force-pushed the remove-end-year-end branch from 00a3101 to 10dd8de Compare February 29, 2024 09:36
Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@xylar Yes, this change seems reasonable to me. I have rebased my branch on this one and it runs without error when endYear != end. Thanks!

@xylar
Copy link
Collaborator Author

xylar commented Feb 29, 2024

Thanks @cbegeman!

@xylar
Copy link
Collaborator Author

xylar commented Mar 14, 2024

@milenaveneziani, do you have time to look this over and let me know if you're okay with it?

@milenaveneziani
Copy link
Collaborator

oops, sorry @xylar that I missed this!
Sounds good to me: I agree that it is better practice to know exactly what data you have and what years you want to perform analysis on.

@xylar
Copy link
Collaborator Author

xylar commented Mar 14, 2024

No worries, thanks @milenaveneziani !

@xylar xylar merged commit 97ab426 into MPAS-Dev:develop Mar 14, 2024
5 checks passed
@xylar xylar deleted the remove-end-year-end branch March 14, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants