-
Notifications
You must be signed in to change notification settings - Fork 168
Double precision #552
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
Double precision #552
Conversation
|
About commit [694c8df] It seems that we had to increase a lot the tolerance to pass the test. It is not really the case. See attached code for a simple example: |
|
There's considerable efforts in the modelling community to explore reduced precision, because this will yield much higher performance on, e.g., GPUs. Just defaulting to double is contrary to this tendency. Would it make sense to consciously handle precision and formulate tolerances in terms of machine precision? |
|
Indeed and we try to avoid using double precision where it's not necessary. Then why this change? It solves a numerical issue. In coastal flows, the particles tend to follow the coastlines, and can be very close to the coast. Impermeable boundary condition of the input data combined with a small time step (~15 minutes) should prevent the particles of beaching, but rounding errors result in beaching particles. By converting lon, lat, depth to double precision, we reduce this un-controlled process by ~95%. So it can have a large effect on the particle general dynamics. How could we solve this problem differently?
Any suggestion on the solution to adopt? |
By default it is single precision for A-grid U field and double precision for C-grid
parcels/particleset.py
Outdated
| :param depth: Optional list of initial depth values for particles. Default is 0m | ||
| :param time: Optional list of initial time values for particles. Default is fieldset.U.grid.time[0] | ||
| :param repeatdt: Optional interval (in seconds) on which to repeat the release of the ParticleSet | ||
| :param coordinates_var_precision: Floating precision for lon, lat, depth particle coordinates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to determine the precision based on the dtype of the lon and lat. Would that not be easier, and more backward-compatible?
The drawback would be that users may still by default use np.float32 for cgrid, but we could add to the nemo-tutorials that we recommend dtype=np.float64 for c-grids?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User generally don't define a dtype to plon and plat. but the dtype is in pset constructor.
- our default way of defining it.
| lon = np.linspace(12000, 21000, npart, dtype=np.float32) | ||
| lat = np.linspace(12500, 12500, npart, dtype=np.float32) | ||
| lon = np.linspace(12000, 21000, npart) | ||
| lat = np.linspace(12500, 12500, npart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does this mean that all user code where dtype is explicitly set now breaks? Is this a reason to determine precision based on the lon/lat dtype?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It was breaking only if we were comparing in python the value of a float with a double
parcels/particleset.py
Outdated
| :param repeatdt: Optional interval (in seconds) on which to repeat the release of the ParticleSet | ||
| :param coordinates_var_precision: Floating precision for lon, lat, depth particle coordinates. | ||
| :param lonlatdepth_dtype: Floating precision for lon, lat, depth particle coordinates. | ||
| It is either 'single' or 'double'. Default is 'single' if fieldset.U.interp_method is 'linear' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we then also rename the options to np.float32 and np.float64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
@erikvansebille this was the PR introducing the lon/lat/depth precision (related to our discussion today about removing this arg and making it a user warning). |
This PR solves a numerical issue that we observe with Parcels.
In coastal flows, the particles tend to follow the coastlines, and can be very close to the coast. Impermeable boundary condition of the input data combined with a small time step (~15 minutes) should prevent the particles of beaching, but rounding errors result in beaching particles. By converting lon, lat, depth to double precision, we reduce this un-controlled process by ~95%.
So it can have a large effect on the particle general dynamics.
How could we solve this problem differently?
Any suggestion on the solution to adopt?