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

fix floating point precision in floatyear_to_date #1747

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

pat-schmitt
Copy link
Member

In this PR I fix a bug which was brought up by @Ruitangtang.

The problem was when calling floatyear_to_date(1+1/12-1/12) you expect to get (1, 1), but actually (0, 12) was returned, due to a floating point precision issue. I now included a check if the given floatyear is inside machine precision to the next higher int, and if so us the next higher int instead.

  • Tests added/passed
  • Fully documented
  • Entry in whats-new.rst

@fmaussion
Copy link
Member

This is a bit of an edge case. Arguably the problem is not in OGGM, since OGGM is given a "wrong" number:

>>> 1 + 1/12 - 1/12
0.9999999999999999

I would normally recommend against doing this kind of arithmetic on "OGGM dates" and use utils.monthly_timeseries, but it's okay to round like this (I hope - the only problem I can foresee is when a tiny time step is needed for the dynamical model - but even then, it's tiny)

@pat-schmitt
Copy link
Member Author

@Ruitangtang is currently implementing a way to save the fl_diagnostics at monthly steps, and for the diagnostic variable climatic_mb we want to use the mb of the previous time-step and hence she is calling self.get_mb(surface_h_previous[fl_id], self.yr - 1/12, fl_id=fl_id), here the current version for yearly timesteps https://github.com/OGGM/oggm/blob/master/oggm/core/flowline.py#L1325.

But probably their is a better way around? Maybe directly saving the previous mb instead of calling it again with -1/12.

And yes, the tiny timestep problem could happen with this method, but it is in the order of 1e-13 of a year (~1e-7 seconds).

Further it seems with numpy 2.0 len(np.array(1)) or len(np.int64(1)) is not working anymore. But len(np.array([1])) us still good.

@fmaussion
Copy link
Member

But probably their is a better way around?

yes I see. I understand why the current behavior is surprising and I agree to the workaround in OGGM. A probable more robust way would be to not to arithmetics on time but rather to do an array of times and use this instead:

monthly_time = utils.monthly_timeseries(y0, y1)
i = 12
print(monthly_time[i])
print(monthly_time[i-1])  # previous months

@pat-schmitt pat-schmitt merged commit 8de5db4 into OGGM:master Oct 22, 2024
27 checks passed
@pat-schmitt pat-schmitt deleted the adapt_float_yr_to_date branch October 22, 2024 07:57
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