Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

avoid implicit conversion to double in VTK I/O routines #255

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

glesur
Copy link
Contributor

@glesur glesur commented Jul 25, 2024

Some compilers (e.g. CLang 17.0) do an implicit conversion to double when using the trigonometric functions. This results in corrupted vtk files since the BigEndian conversion and the VTK format all expect single precision coordinates in VTK files.

This PR fixes this issue encountered recently with Rocm 5.7

Copy link
Collaborator

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Would it be possible to fix this on the BigEndian function instead ? This would avoid having to also fix other call sites (I'm thinking maybe bout the private particles module). If not, I can apply a similar patch to the private module.

@glesur
Copy link
Contributor Author

glesur commented Jul 25, 2024

The only fix possible with bigEndian only is to restrict the input type to float only.

Incidentally, I don't recall why this function was generalised as a template function... (which is bugged for any type that is not coded on 4 bytes). Clearly, this mess should be cleaned up.

@neutrinoceros
Copy link
Collaborator

I don't recall why this function was generalised as a template function

I don't recall either, but it seems fishy. Anyway, no big deal if you don't want to take the risk to undo the templating.

- works for any type size
- checks that we are dealing with numbers
- remove pointers in favor of more elegant c++ unions
@glesur
Copy link
Contributor Author

glesur commented Jul 26, 2024

templating is needed because we need big endian conversion for both floats and integer (for the geometry and periodicity flags you introduced back in the days)

Just pushed a new version that implements bigendian conversion in an agnostic manner (i.e. doesn't assume anything regarding the size of the input type). While this is now full-proof, it doesn't prevent the original bug which was an implicit conversion to double precision before calling BigEndian, because the sin and cos functions in C are normally returning double precision.

In other words, if some sin and cos functions are used in the VTK for particles, these should be replaced by their c++ equivalent std::sin and std::cos, that are properly overloaded for single precision inputs.

@glesur glesur merged commit 0a6193d into develop Jul 26, 2024
13 checks passed
@glesur glesur deleted the fixEndiannessVTK branch July 26, 2024 15:05
@glesur glesur mentioned this pull request Oct 23, 2024
glesur added a commit that referenced this pull request Oct 24, 2024
## [2.1.02] 2024-10-24
### Changed

- Fix a bug that could lead to corrupted VTK file when using single precision arithmetic (#255)
- Fix a bug that could lead to incorrect central mass gravitational potential upon restart (#287)
- Changed the way magnetic field is reconstructed when using grid coarsening to reduce roundoff errors on div(B). This can have an impact on the results of models using grid coarsening+MHD (#284)
- Ensure that XDMF outputs are precision agnostic (#261)
- Bump up Kokkos version to 4.4.01 (#289)
- Check that writes are successfull in serial, otherwise throw an error (#260)
- Ensure that shock flattening flags can be modified by user (#260)
- Throw an error when user enables Fargo without enough DIMENSIONS (#250)
- Fix linting errors following upgrade to cpplint 2.0 (#278, #279, #281)
- Update idfx_io to numpy 2.0 (#283)

### Added

- Allow the user to define the grid and boundary conditions only on active dimensions (#274)
- Configuration for Nvidia H100 on Jean Zay in the documentation
---------

Co-authored-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
Co-authored-by: vdbma <93188557+vdbma@users.noreply.github.com>
Co-authored-by: Marc Van den Bossche <marc.vanden-bossche@univ-grenoble-alpes.fr>
Co-authored-by: Alankar Dutta <dutta.alankar@gmail.com>
Co-authored-by: Alankar Dutta <alankard@MB-167.local>
Co-authored-by: ThomasJannaudCAM <159052976+ThomasJannaudCAM@users.noreply.github.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: marc <vandenbossche.marc@hotmail.com>
Co-authored-by: Antonin Borderies <89980449+Anto6453@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants