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

MutVector<F>#replace() problem #46

Closed
axkr opened this issue Dec 9, 2021 · 5 comments
Closed

MutVector<F>#replace() problem #46

axkr opened this issue Dec 9, 2021 · 5 comments
Labels

Comments

@axkr
Copy link

axkr commented Dec 9, 2021

MutVector<F>#replace()does not replace the value at index 1 in this case:

  • image

Also the question is the replace() method the right way to replace a value in a vector?

@GlenKPeterson
Copy link
Owner

GlenKPeterson commented Dec 9, 2021

This unit test passes:

    @Test public void testMutable() {
        List<Integer> control = new ArrayList<>();
        MutList<Integer> test = PersistentVector.emptyMutable();
        final int SEVERAL = 2000; // more than 1024 so 3 levels deep.
        for (int i = 0; i < SEVERAL; i++) {
            control.add(i);
            test.append(i);
            assertEquals(control.size(), test.size());
        }

        for (int i = 0; i < SEVERAL; i++) {
            assertEquals(control.get(i), test.get(i));
        }

        for (int i = 0; i < SEVERAL; i++) {
            control.set(i, i + 10);
            test.replace(i, i + 10);
            assertEquals(control.size(), test.size());
            assertEquals(control, test);
        }

Can you alter it to make it fail? If you can do that, you don't need to answer any of my other questions. Otherwise:

If I understand what I'm looking at in your image, you have a mutable vector with a list as the first element and Doubles or Floats as the other visible elements. You just called replace with object id=314 and idx=1 and it looks like it put object id=314 (-92.4031) at index 1 and is ready to return the correctly mutated-in-place vector.

Questions about your example:

  • What was at index 1 before?
  • What was your expected result, vs. the actual?
  • Why is there a List at index 0 when everything else in the vector looks like double? I don't use a lot of Heterogeneous vectors, so I guess this is more of a chatty question than troubleshooting.

Questions about reproducing this issue:

  • How big a vector do you need to see the problem?
  • Does it happen with homogeneous vectors, or just heterogeneous ones?
@axkr
Copy link
Author

axkr commented Dec 12, 2021

Seems to be a problem with the MutVector#replace() and call of MutVector#editableArrayFor() method.

Example for 3.6.0 version from Maven Central:

import org.organicdesign.fp.StaticImports;
import org.organicdesign.fp.collections.ImList;
import org.organicdesign.fp.collections.MutList;

public class PersistentVectorTest {
  public static void main(String[] args) {
    ImList<String> v = StaticImports.vec("0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10",
        "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25",
        "26", "27", "28", "29", "30", "31", "32", "33", "34", "35", "36", "37", "38", "39", "40",
        "41", "42", "43", "44", "45", "46", "47", "48", "49", "50", "51", "52", "53", "54", "55",
        "56", "57", "58", "59", "60", "61", "62", "63", "64", "65", "66", "67", "68", "69", "70",
        "71", "72", "73", "74", "75", "76", "77", "78", "79", "80", "81", "82", "83", "84", "85",
        "86", "87", "88", "89", "90", "91", "92", "93", "94", "95", "96", "97", "98", "99");
    MutList<String> m = v.mutable();
    m.replace(0, "bug");
    System.out.println(m.get(0).toString());
  }
}
@GlenKPeterson
Copy link
Owner

Thanks for finding this bug! I will fix, but maybe not until next weekend.

GlenKPeterson added a commit that referenced this issue Dec 19, 2021
…the node it appeared to because it needed to replace the reference to the node, not the value at the node. This must not ever get called in the Clojure implementation because it's copied straight from there. No idea the performance implications of this change, but I suspect small? Thanks to Axel Kramer @axkr for finding and reporting this bug #46.
@GlenKPeterson
Copy link
Owner

Thank you so much! This should be fixed in 3.7.0 which I just released. If you get a chance to test it, please leave a comment here and I'll close the issue.

axkr added a commit to axkr/symja_android_library that referenced this issue Dec 20, 2021
@axkr
Copy link
Author

axkr commented Dec 20, 2021

Seems to be fixed

@axkr axkr closed this as completed Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants