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

Remove redundant "tail" argument from memmap loading #1219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oerc0122
Copy link
Collaborator

@oerc0122 oerc0122 commented Sep 7, 2023

No description provided.

@oerc0122 oerc0122 added the Technical Debt code quality, unit tests, code duplicaton label Sep 7, 2023
@oerc0122 oerc0122 self-assigned this Sep 7, 2023
@oerc0122 oerc0122 force-pushed the remove-unnecessary-tail branch from cfecf4b to 9c330ba Compare September 7, 2023 10:05
Copy link
Member

@abuts abuts left a comment

Choose a reason for hiding this comment

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

This change is incorrect and harmful.
As soon as pixels are last dataset in sqw file, all this works. As soon as you write something after pixels when updating some datasets, it fails with very strange error if tail is not present.

It is of course very bad that unit tests do not cach this change, though I believe I was writing some tests for that.
This ticket may be modified to write unit tests which check tail presence necessary

@oerc0122
Copy link
Collaborator Author

This change is incorrect and harmful. As soon as pixels are last dataset in sqw file, all this works. As soon as you write something after pixels when updating some datasets, it fails with very strange error if tail is not present.

It is of course very bad that unit tests do not cach this change, though I believe I was writing some tests for that. This ticket may be modified to write unit tests which check tail presence necessary

That sounds like it's masking some other error and we should definitely not have this. Retry since I've rewritten the PixelData dumpers.

@abuts
Copy link
Member

abuts commented Sep 14, 2023

I've observed failure when tail is not there more then once. You've already tried to remove tail, and I have to restore it as tests started to fail randomly. The test, which writes something after pixels and shows that you can still read pixels without tail is absolutely necessary to prove you can remove it. It is test for memmapfile rather then for anything else

@oerc0122
Copy link
Collaborator Author

And thus it is clearly masking a deeper problem. This is not a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Technical Debt code quality, unit tests, code duplicaton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants