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

Class cast exception when parsing metadata #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dgault
Copy link
Member

@dgault dgault commented Aug 30, 2024

This PR is a follow up to #94
I have simply taken the 2 commits from the fork by @jo-mueller : https://github.com/jo-mueller/ZarrReader/tree/fix-classcast-exception

Fixes #94

…e conversion properly.

* Use `Number` type to handle both `Integer` and `Double`.
* Convert `Number` to `Double` using `Number.doubleValue()`.

Add test cases in `ZarrReaderTest.java` to verify the fix for `parseOmeroMetadata`.

* Add test cases to cover both `Integer` and `Double` values in the metadata.
…ferent types of numeric values correctly

* Check the type of the value before casting it to `Double`
* Convert `Integer` values to `Double` before further processing
@jo-mueller
Copy link

Cool! If that fixes the error, could you include me as a co-author of the commits from my fork? Or did you copy the commits altogether?

@dgault
Copy link
Member Author

dgault commented Aug 30, 2024

Looks like the tests are failing to build due to parseOmeroMetadata being a private method.

@jo-mueller, you can certainly be added to the list of contributors if this change gets included. We have a Contributor License Agreement for the project, would you be able to sign and return the form following the instructions on https://ome-contributing.readthedocs.io/en/latest/cla.html

@jo-mueller
Copy link

Hi @dgault , thanks for moving this forward. I already signed the OME agreeement for this PR: ome/omero-py#394

@joshmoore
Copy link
Member

@jo-mueller: 👍 on the signing. A heads up in case you haven't seen https://forum.image.sc/t/bio-formats-support/101109, though, you might want to re-open this under your own account.

@jo-mueller
Copy link

Hi @joshmoore,

thanks for the heads-up. I could open a PR with the two commits myself, the issue with that would be that I myself will be on parental leave until December starting the coming Friday. I could probably greenlight any changes to the PR from the mobile app but I think t would be better if somehow could see this through with more regular access to their devices :/

@joshmoore
Copy link
Member

Understood, and enjoy! If this hasn't made it in by the time you're back, feel free to speak up loudly.

@jo-mueller
Copy link

Understood, and enjoy! If this hasn't made it in by the time you're back, feel free to speak up loudly.

I'll hold you to it at next year's TiM conference latest if I meet you there ;)

@dominikl dominikl mentioned this pull request Dec 5, 2024
@dominikl
Copy link
Member

dominikl commented Dec 5, 2024

Could be closed in favor of #100 ?

@joshmoore
Copy link
Member

@dominikl : I was going to say that merging your PR will close this one, but I'm not sure that's the case since it looks like the cherry-pick information isn't there (original author, etc.)

Edit: ah, now I see #100 (comment)

@dominikl
Copy link
Member

dominikl commented Dec 5, 2024

Maybe I shouldn't have split the commit? Shall I revert it? But it needed another little fix too. Don't know what's the best way to handle that.

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.

java.lang.ClassCastException on ZARR import
4 participants