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

bug fix, thumb nails are unsigned integers, and exceeded 2^31, so I changed their size to Long #4100

Closed
wants to merge 8 commits into from

Conversation

nicolapapp
Copy link
Contributor

@nicolapapp nicolapapp commented Oct 5, 2023

There were some large numbers (> 2^31) in the yaml metadata files which referred to the older sld format pointers in the file, and are not used in format 7 (sldy). These numbers anyway were read in as Integer and that generated an exception. I have tracked down several and changed them to Long. I have also added try/catch around the decoding of numbers from the yaml file, and that will produce warnings but will not kill the file load. If during testing you get any warnings in the console, please send them to me.

nicolapapp and others added 7 commits October 24, 2022 11:14
added the code for getUsedFile
added support for compress sldyz/npyz files
Fixed GetVoxelSize
thumb nails are unsigned integers, and exceeded 2^31, so I changed their size to Long
… 2^31

fixed more errors when trying to decode an integer value greater than 2^31
@nicolapapp
Copy link
Contributor Author

There were some large numbers (> 2^31) in the yaml metadata files which referred to the older sld format pointers in the file, and are not used in format 7 (sldy). These numbers anyway were read in as Integer and that generated an exception. I have tracked down several and changed them to Long. I have also added try/catch around the decoding of numbers from the yaml file, and that will produce warnings but will not kill the file load. If during testing you get any warnings in the console, please send them to me.

@dgault dgault added the include label Oct 6, 2023
@snoopycrimecop
Copy link
Member

Empty PR description. Please add a short summary of the PR scope and some testing instructions.

5 similar comments
@snoopycrimecop
Copy link
Member

Empty PR description. Please add a short summary of the PR scope and some testing instructions.

@snoopycrimecop
Copy link
Member

Empty PR description. Please add a short summary of the PR scope and some testing instructions.

@snoopycrimecop
Copy link
Member

Empty PR description. Please add a short summary of the PR scope and some testing instructions.

@snoopycrimecop
Copy link
Member

Empty PR description. Please add a short summary of the PR scope and some testing instructions.

@snoopycrimecop
Copy link
Member

Empty PR description. Please add a short summary of the PR scope and some testing instructions.

@dgault dgault removed the include label Oct 9, 2023
@dgault
Copy link
Member

dgault commented Oct 9, 2023

@nicolapapp This PR seems to have caused some failures in our nightly repository tests, https://merge-ci.openmicroscopy.org/jenkins/job/BIOFORMATS-test-folder/lastFailedBuild/console

It looks as though some files are now failing isThisType and not being recognised by the reader, and for others the physical pixel size seems to have changed:

[testng] [2023-10-09 01:41:29,317] [pool-1-thread-1] 	PhysicalSizeX: FAILED (Series 0 (expected ome.units.quantity.Length: value[1.0], unit[µm] stored as java.lang.Double, actual ome.units.quantity.Length: value[0.10599999874830246], unit[µm] stored as java.lang.Double))
 [testng] [2023-10-09 01:41:29,318] [pool-1-thread-1] 	PhysicalSizeY: FAILED (Series 0 (expected ome.units.quantity.Length: value[1.0],

…taStore

Added code to import the ROI annotations and store them in the MetadataStore. tested ok in fiji by me and out slidebook project manager
@dgault
Copy link
Member

dgault commented Nov 15, 2023

@nicolapapp, there is a large number of code changes included here that seem to be greater than the original description of the PR (changing unsigned ints to long). Are all of these changes meant to be included? Do you have a description of what other functionality has changed and how we can best test it?

@melissalinkert
Copy link
Member

@nicolapapp : in order to consider this pull request for inclusion, we will need answers to the questions in #4100 (comment). If we don't hear back from you within the next week, this pull request will be closed (but you are welcome to re-open it once the outstanding questions are answered).

@nicolapapp
Copy link
Contributor Author

There were some large numbers (> 2^31) in the yaml metadata files which referred to the older sld format pointers in the file, and are not used in format 7 (sldy). These numbers anyway were read in as Integer and that generated an exception. I have tracked down several and changed them to Long. I have also added try/catch around the decoding of numbers from the yaml file, and that will produce warnings but will not kill the file load. If during testing you get any warnings in the console, please send them to me.

@melissalinkert
Copy link
Member

There were some large numbers (> 2^31) in the yaml metadata files which referred to the older sld format pointers in the file, and are not used in format 7 (sldy). These numbers anyway were read in as Integer and that generated an exception. I have tracked down several and changed them to Long. I have also added try/catch around the decoding of numbers from the yaml file, and that will produce warnings but will not kill the file load. If during testing you get any warnings in the console, please send them to me.

Thanks @nicolapapp. I understand that is the intention of the first few commits here, but there are other changes (for example 0fd31bd) that are not related to large number handling. Could you please either remove those other changes, or summarize what they are and let us know how we can test them?

@melissalinkert
Copy link
Member

Closing as noted above. @nicolapapp, feel free to re-open this pull request once the extra commits are removed, or the full set of changes is described along with appropriate test data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants