Skip to content

Updated handling of empty arrays for math operators - #1296

Open
alexibrooks wants to merge 2 commits into
Doenet:mainfrom
alexibrooks:EmptyListHandling
Open

Updated handling of empty arrays for math operators#1296
alexibrooks wants to merge 2 commits into
Doenet:mainfrom
alexibrooks:EmptyListHandling

Conversation

@alexibrooks

Copy link
Copy Markdown

Previously, MathBaseOperator had a default behavior to return "\uff3f" in the event of a zero-length input, which led to the odd behavior of <count></count> not returning the expected 0. New behavior (first number is forceNumeric, second is forceSymbolic):

image

To replicate the image above, use:

<p>Sum: <sum forceNumeric></sum> <sum forceSymbolic></sum> </p>
<p>Product: <product forceNumeric></product><product forceSymbolic></product></p>
<p>Mean: <mean forceNumeric></mean><mean forceSymbolic></mean></p>
<p>Median: <median forceNumeric></median><median forceSymbolic></median></p>
<p>Variance: <variance forceNumeric></variance><variance forceSymbolic></variance></p>
<p>StandardDev: <standardDeviation forceNumeric></standardDeviation><standardDeviation forceSymbolic></standardDeviation></p>
<p>Count: <count forceNumeric></count><count forceSymbolic></count></p>
<p>Min: <min forceNumeric></min><min forceSymbolic></min></p>
<p>Max: <max forceNumeric></max><max forceSymbolic></max></p>
<p>Mod: <mod forceNumeric></mod><mod forceSymbolic></mod></p>
<p>Gcd: <gcd forceNumeric></gcd><gcd forceSymbolic></gcd></p>
<p>LCM: <lcm forceNumeric></lcm><lcm forceSymbolic></lcm></p>

First time contributor - I'm happy to additional testing legwork but I will need a little guidance about what needs to be done.

@dqnykamp dqnykamp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, thanks for you PR. The implementation looks good to me.

Could you run prettier to fix the Lint CI?

npm run prettier:format

The one missing piece is adding tests for the new behavior. Could you add a test at the end of mathoperators.test.ts? You could copy an existing test, like the last one, and replace the doenetML with your example from the PR description. You'll need to add a name for each of those components. Then, once the stateVariables object is created, you can create assertions for the value state variable.

For example, if I make the first DoenetML line be

<p>Sum: <sum forceNumeric name="sumNumeric"></sum> <sum forceSymbolic name="sumSymbolic"></sum> </p>

then the appropriate assertions that the sum is zero would be

        expect(
            stateVariables[await resolvePathToNodeIdx("sumNumeric")].stateValues
                .value.tree,
        ).eq(0);
        expect(
            stateVariables[await resolvePathToNodeIdx("sumSymbolic")]
                .stateValues.value.tree,
        ).eq(0);

Unfortunately, if you forget the .tree, it gives a cryptic error message that doesn't even indicate on which line the error occurs.

The tests are really long, so I usually put a .only on the new test, e.g., it.only("test name", ... and then just run the one test file by going to the packages/doenetml-worker-javascript directory and running

npm run test

in that directory.

The CI tests should fail if you leave the .only there, so be sure to remove it before committing.

@alexibrooks

Copy link
Copy Markdown
Author

Working on this. Any suggestions on how to test for the expected __ values? I can do something like this I think:

expect(
            stateVariables[await resolvePathToNodeIdx("symbolicEmpty")]
                .stateValues.isNumber,
        ).eq(false);

But there should be a way to be more specific. I'm not there yet, but I'm also concerned about how to test for "min()" etc.

@dqnykamp

Copy link
Copy Markdown
Member

Working on this. Any suggestions on how to test for the expected __ values? I can do something like this I think:

You can compare either value.tree or latex against the unicode fullwidth low line character, "\uff3f".

        expect(
            stateVariables[await resolvePathToNodeIdx("symbolicEmpty")]
               .stateValues.value.tree,
        ).eq("\uff3f");

@dqnykamp

Copy link
Copy Markdown
Member

I'm not there yet, but I'm also concerned about how to test for "min()" etc.

A handy tool is to inspect the state in the Javascript console. For the DoenetML <min name="m" forceSymbolic />, three components get created: a <document>, a <min>, and a <boolean> (for the value of forceSymbolic). To see them, you can enter returnAllStateVariables1(), which I think is the same as core.returnAllStateVariables() used in the tests. (It's called with slightly different arguments, so the .trees are gone.)

image

If you open up the information about the min
image
you can see what the resulting value is:
image

Apparently, it is applying the min function to an empty tuple. So you could assert

        expect(
            stateVariables[await resolvePathToNodeIdx("m")]
               .stateValues.value.tree,
        ).eqls(["apply", "min", ["tuple"]]);

where using .eqls() rather than .eq() is important given the way Javascript treats two arrays with the same values as unequal unless they are the exact same object.

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.

2 participants