GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — DefaultConverter has many broken cases · Issue #450 · scijava/scijava-common

DefaultConverter has many broken cases #450

New issue
Copy link
New issue
Copy link
Closed
Closed
DefaultConverter has many broken cases#450
Copy link

Description

@ctrueden
ctrueden
opened on Feb 1, 2023
Issue body actions

The DefaultConverter was not being tested directly. With 4e40e04, now it is. But there are many cases that do not work, marked with commented out tests. (Also many missing tests, but at least it's a start!)

This issue exists to document some of the known problems, and thoughts on how best to resolve them.

Just to give an inkling of the problems highlighted by this issue:

--- a/src/main/java/org/scijava/convert/DefaultConverter.java
+++ b/src/main/java/org/scijava/convert/DefaultConverter.java
@@ -73,6 +73,7 @@ public class DefaultConverter extends AbstractConverter<Object, Object> {
 
        @Override
        public Object convert(final Object src, final Type dest) {
+               if (!canConvert(src, dest)) return null;
 
                // Handle array types, including generic array types.
                final Type componentType = Types.component(dest);
@@ -92,6 +93,8 @@ public class DefaultConverter extends AbstractConverter<Object, Object> {
 
        @Override
        public <T> T convert(final Object src, final Class<T> dest) {
+               if (!canConvert(src, dest)) return null;
+
                // ensure type is well-behaved, rather than a primitive type
                final Class<T> saneDest = Types.box(dest);

☝️ This patch breaks three tests. 😮

So let's consider the following questions:

What would you expect to happen in the following circumstances? You ask SciJava Common to convert some data you have, to a particular type:

  1. "{{0, 1}, {2, 3}}" → double[][]. Do you expect: A) [[0d, 1d], [2d, 3d]]? B) null?
  2. "{{0, 1}, {curtis, 3}}" → Double[][]. Do you expect: A) [[0d, 1d], [null, 3d]]? B) null?
  3. "{{0, 1}, {curtis, 3}}" → double[][]. Do you expect: A) [[0d, 1d], [NaN, 3d]]? B) null?
  4. "5" → double[]. Do you expect: A) [5d]? B) null?
  5. "5" → double[][]. Do you expect: A) [[5d]]? B) null?
  6. "curtis" → double[]. Do you expect: A) [NaN]? B) null?
  7. "curtis" → Double[]. Do you expect: A) [null]? B) null?
  8. "curtis" → Double[][]. Do you expect: A) [[null]]? B) null?

I think the following behavior is what we should shoot for:

  • When converting to an array/collection type, we first go to Object[], Object[][], etc., of correct dimensionality.
  • If dimensionality of this initially converted source is less than dimensionality of target, we wrap it in extra dimensions till it's the same.
  • If dimensionality of the initially converted source is more than the dimensionality of target, return null.
  • Otherwise: we allocate the destination object of the requested type, then convert source's leaf elements one by one to destination elements.

E.g. for (8) above, we would go "curtis" → new Object[][] {{"curtis"}} → new Double[][] {{ convert("curtis", Double.class) }}.

The remaining question is: what to do if any/all of those leaves are not convertible?

After some discussion between myself, @hinerm, and @gselzer, we narrowed down the following potentially reasonable behavior:

  1. If all of the leaf elements are convertible to the destination element type, then the converted object is populated. Otherwise, the whole collection is inconvertible, and we return null for the whole collection, and false for canConvert. This would answer the above list with null for questions 2, 3, 6, 7, 8.

  2. If any of the leaf elements are convertible to the destination element type, then the converted object is populated, filling any inconvertible elements with null. This would answer the above list with null for questions 6, 7, 8—but 2 and 3 would work.

  3. Regardless of whether any leaf elements are convertible to the destination element type, the converted object is populated, filling nulls as in (2). This would answer the above list with "As all the way down" i.e. no null results.

Part of this issue is deciding which of these three behaviors would be best. Then fixing the DefaultConverter to work that way—in both its canConvert and convert methods—and uncommenting the disabled tests (and adjusting them as needed) to validate it.

Reacted by Gabriel Selzer and Jan Eglinger

Activity

  1. gselzer commented on Feb 2, 2023

    @gselzer
    gselzer
    on Feb 2, 2023
    Member
    More actions

    It would be cool if, through solving this PR, we could formalize 1 or 2 or 3 as the intent of the ConvertService, although I admit that may be infeasible for SciJava Common if any consensus would be a breaking change.

  2. maarzt
    added a commit that references this issue on Feb 10, 2023

    Add unit tests for DefaultConverter

    4e40e04
  3. ctrueden
    added 7 commits that reference this issue on Feb 24, 2023

    DefaultConverterTest: add new test for #450 cases

    d719763

    DefaultConverterTest: add new test for #450 cases

    71aa64d

    WIP: FIX EVERYTHING FOREVER

    39dbaf4

    WIP: DefaultConverterTest: add tests for 2D arrays

    c99737d

    WIP: StringToArrayConverterTest: test #450 cases

    5379a12

    StringToArrayConverter: fix decoding of parse tree

    bf0ef49

    StringToArrayConverter: fix decoding of parse tree

    ce135b7
  4. ctrueden
    closed this as completedin ce135b7on Mar 2, 2023
  5. imagesc-bot commented on Jul 13, 2023

    @imagesc-bot
    imagesc-bot
    on Jul 13, 2023
    More actions

    This issue has been mentioned on Image.sc Forum. There might be relevant details there:

    https://forum.image.sc/t/change-of-behaviour-in-scijava-parameter-table-of-size-one-with-null-object-inside-instead-of-null-object/83625/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions

      You are visitor 01402503 — GeezerVison does not store the pages you look up.
      Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.