search

payu-org / payu

A workflow management tool for numerical models on the NCI computing systems
Apache License 2.0
18 stars 25 forks source link

Changed to floating point division in MITgcm to support fractional timestep #260

Closed aidanheerdegen closed 4 years ago

aidanheerdegen commented 4 years ago

Changed to floating point division in MITgcm to avoid issues with floating point round off with fractional timesteps.

Added mitgcm model tests.

Fixes #228

pep8speaks commented 4 years ago

Hello @aidanheerdegen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-05-14 02:15:33 UTC
aekiss commented 4 years ago

@aidanheerdegen are you sure you want n_timesteps and n_iter0 to be floats? They aren't guaranteed to be whole numbers.

aidanheerdegen commented 4 years ago

@aidanheerdegen are you sure you want n_timesteps and n_iter0 to be floats? They aren't guaranteed to be whole numbers.

It's a good point. Mostly I wanted to make some tests so I could change the MITgcm driver to support fractional time steps and not break existing functionality.

I guess that is why I had the // divisions to begin with. I'll look into forcing those variables to be integer. Tah.

aekiss commented 4 years ago

I guess if MITgcm supports fractional timesteps it would be best to do whatever MITgcm does, at the same numerical precision? I think python uses 64-bit double precision by default.

aidanheerdegen commented 4 years ago

MITgcm supports arbitrary model time steps, but clearly the number of time steps is an integer.

I'm not attempting to cover all possible use cases, more coding to the tests, which I'll take as a reasonable representation of usage. If payu breaks because of different use cases then I'll look at adding those to the tests and changing the code appropriately. Otherwise it is pretty easy to lose your mind and a great deal of time.

aidanheerdegen commented 4 years ago

Whaddya reckon @aekiss? Missed anything obvious?

aekiss commented 4 years ago

Does n_timesteps need to be the actual number of timesteps that MITgcm will take?

If MITgcm simply adds dt to t_start over and over until time exceeds t_end it could give a different number of iterations from your rounded answer when dt is a float, depending on whether the summation rounds high (reducing the iterations by one) or low (increasing number of iterations). (In the worst case (model time being much larger than dt), maybe MITgcm could fail to increment model time at all due to catastrophic roundoff and get in an endless loop?)

Anyway if you need to match the number of MITgcm timesteps, the surest way would be to do the summation in the same way as MITgcm, to the same precision, and count the number of iterations. Crazy I know, but I can't think of a better way.

Incidentally I think this sort of hassle is one reason that numy.arange excludes the limiting value.

aekiss commented 4 years ago

Also if it were me I wouldn't force t_start and t_end to be floats. If you leave them as int you can have roundoff-free integer arithmetic in the (probably most common) case of integer dt.

aidanheerdegen commented 4 years ago

Thanks. I don't like the idea of the code behaving in a different way depending on the implicit type of a number that it reads in. I think I'd prefer it to be consistent and fail, consistently. But point taken.

As for copying what MITgcm does, not going there unless it fails. It is just a driver program. If it fails it should do so loudly and then it can be fixed.

I'll merge this as Jim is waiting for a fix.