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.

Sign up for GitHub

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

Jump to bottom

Overflow issues in Vol/Longvol readers #1637

Merged
merged 13 commits into from Mar 21, 2022
Merged

Overflow issues in Vol/Longvol readers #1637

merged 13 commits into from Mar 21, 2022

Conversation

dcoeurjo
Copy link
Member

@dcoeurjo dcoeurjo commented Mar 17, 2022 •
edited

PR Description

Overflow issues in Vol/Longvol readers on large domains. This pr also fixes issues with azure build bots.

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug mode.
  • All continuous integration tests pass (Github Actions & appveyor)

@dcoeurjo dcoeurjo changed the title overflow issues in Vol/Longvol readers Overflow issues in Vol/Longvol readers Mar 17, 2022
@dcoeurjo dcoeurjo requested a review from kerautret March 17, 2022 14:19
@rolanddenis
Copy link
Member

Can this be related to the failures on Github Actions, with testLongvol and testCompressedVolWriter, you talk about few weeks ago (#1630 (comment)) ?

@dcoeurjo
Copy link
Member Author

Han... maybe.. let me pull the tests out from the "blacklist"..

@dcoeurjo
Copy link
Member Author

Nope.. maybe related but this PR does not fix the weird bug on the macOS bot (fine on mine)

@rolanddenis
Copy link
Member

Actually, I've just reproduced a similar bug on my computer by using a higher value in testCompressedVolWriter at https://github.com/DGtal-team/DGtal/blob/master/tests/io/writers/testCompressedVolWriter.cpp#L96 . The test fails with the same read value as in Github Actions (BTW it is the maximal value for uint64). I think it is due to a left-shift overflow in LongvolReader, I will take a look next week !

kerautret
kerautret approved these changes Mar 20, 2022
View reviewed changes
Copy link
Member

@kerautret kerautret left a comment

Choose a reason for hiding this comment

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

just update the changelog #PR suggest

ChangeLog.md Outdated Show resolved Hide resolved
ChangeLog.md Outdated Show resolved Hide resolved
wrap/deploy/deploy_macos_one_python.sh Outdated Show resolved Hide resolved
wrap/deploy/manylinux-build-wheels.sh Outdated Show resolved Hide resolved
dcoeurjo and others added 4 commits March 21, 2022 08:58
@dcoeurjo @kerautret
Update ChangeLog.md
5388a63 
Co-authored-by: Bertrand Kerautret <bertrand.kerautret@univ-lyon2.fr>
@dcoeurjo @kerautret
Update ChangeLog.md
f0be74d 
Co-authored-by: Bertrand Kerautret <bertrand.kerautret@univ-lyon2.fr>
@dcoeurjo @kerautret
Update wrap/deploy/deploy_macos_one_python.sh
70afab3 
Co-authored-by: Bertrand Kerautret <bertrand.kerautret@univ-lyon2.fr>
@dcoeurjo @kerautret
Update wrap/deploy/manylinux-build-wheels.sh
0969ce4 
Co-authored-by: Bertrand Kerautret <bertrand.kerautret@univ-lyon2.fr>
@dcoeurjo
Copy link
Member Author

ythx @kerautret

@dcoeurjo dcoeurjo merged commit 440aafd into master Mar 21, 2022
@dcoeurjo dcoeurjo deleted the LargVol branch March 21, 2022 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerautret kerautret kerautret approved these changes

@JacquesOlivierLachaud JacquesOlivierLachaud Awaiting requested review from JacquesOlivierLachaud

Assignees
No one assigned
Labels
IO-Visualisation ready for review 👍
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dcoeurjo @rolanddenis @kerautret