maths / moodle-qtype_stack

Stack question type for Moodle
GNU General Public License v3.0
142 stars 149 forks source link

Floating point numbers not displaying correctly in the validation field #649

Closed lassikor closed 7 months ago

lassikor commented 4 years ago

Hi, we have noticed the following problem in the validation display. If the answer includes more than one floating point numbers, the validation displays all the numbers using the same number of decimal places as used in the first given floating point. This behaviour can be clearly seen on the pictures below. The validation of the input can be quite confusing at the point of student's view and I believe that the validation is not supposed to work like this. BR Lassi Korhonen numbers1 numbers2 numbers3

aharjula commented 4 years ago

Chris, seems like a place for filter number 910 and probably ties into the things you are currently working on as well...

mariusros commented 1 year ago

Hello, is there a solution to this "problem" yet? We're running into the same issue.

aharjula commented 1 year ago

No. There have been too many other things to fix and develop. Overall, this issue connects with a larger refactoring of the input system. While prototypes of the new system provide the desired behaviour, the old one functions in a vastly different way, so mapping that to the old one is not easy.

It might be that someone gets inspired and finds the time to modify the old system, but to me, it is more likely that the new system will eventually fix this, but not anytime soon. Basically, the issue is that the old system uses the validation process to also produce the displayed version, and that process is not really checking more than the first floats presentation settings and then applies that to all floats. It would need to separate the display processing from the validation processing to keep tracking those settings on a float by float basis, which is essentially what the above-mentioned filter 910 does. The new system separates display and validation into strictly different things and is willing to waste CPU cycles to improve both, the old one goes back over a decade and is much more frugal and tries to do both at the same time and thus we end up with this behaviour.

So, in the end, this issue, like many others, is tied to a long multi-year project for replacing the whole input system, and while it is already fixed in that project, the project is not merging into the release version yet. The reason for not merging is mainly due to the new input system breaking compatibility and increasing the complexity related to authoring related to the options of inputs and will thus need much more thought. We need new authoring tools to deal with new options that have more structure. Nevertheless, many of the massive upgrades to the STACK infrastructure that have happened during the last three years have been done to set up the stage for that system and we are getting there although slowly.

m-r-k commented 1 year ago

I would like to add the following strange behavior: pi compared to e

aharjula commented 1 year ago

Well, that is a mystery... It only happens with e or E present before a float, but it is not coming from the parser level as it seems to be able to parse that just fine:

== = testing = e+4.3^k = ==
The test-string was directly parseable.
The AST is like this before filters:
([Root] ([Op: +] ([Id] e), ([Op: ^] ([Float] 4.3), ([Id] k))))
e+4.3^k
------- MP_Root
------- MP_Statement
------- MP_Operation +
-       MP_Identifier e
  ----- MP_Operation ^
  ---   MP_Float 4.3
      - MP_Identifier k

And it is not an artifact created by the corrective parser, which we can trigger if we add 2x into that expression:

== = testing = e+4.3^k+2x = ==
The test-string was not directly parseable, some corrections required.
The AST is like this before filters:
([Root] ([Op: +] ([Id] e), ([Op: +] ([Op: ^] ([Float] 4.3), ([Id] k)), ([Op: *] ([Int] 2), ([Id] x)))))
e+4.3^k+2*x
-------------MP_Root
----------- MP_Statement
----------- MP_Operation +
-           MP_Identifier e
  --------- MP_Operation +
  -----     MP_Operation ^
  ---       MP_Float 4.3
      -     MP_Identifier k
        --- MP_Operation * [insertstars]
        -   MP_Integer 2
          - MP_Identifier x

So something must be happening in the filters, but I don't see any singular filter doing that.

Based on what goes into the CAS and what comes out, this must happen in the CAS. And one of the places where this can happen is this:

(%i15) simp:false;
(%o15) false
(%i16) stack_disp(%e+4.3^k,"");
(%o16) "e+4.3e+0^{k}"
(%i17) simp:true;
(%o17) true
(%i18) stack_disp(%e+4.3^k,"");
(%o18) "4.3e+0^{k}+e"

The CAS cache shows us that the expression is all fine when going in, and its value is still fine coming out, but the LaTeX presentation is broken. So an issue in the handling of rendering the floats, but where is the logic for tying that e to the issue? Somewhere the whole expression must be processed as a whole thing and not just a single float rendered as a singular thing.

aharjula commented 1 year ago

Found it:

(%i23) stackfltfmt:"~a";
(%o23) "~a"
(%i24) stack_disp(%e+4.3^k,"");
(%o24) "e+4.3^{k}"
(%i25) stackfltfmt:"~e";
(%o25) "~e"
(%i26) stack_disp(%e+4.3^k,"");
(%o26) "e+4.3e+0^{k}"

And that format is being defined for the input as a whole. The logic there is faulty because it thinks that the e has any role for that float. On the other hand, it could not work if mixed format floats were present, so solving this properly would again require targeting format at float by float level using that filter 910.

Basically, the issue is that a bit of logic, meant for numerical inputs with singular numerical values as the first thing in the input, is being applied to expressions with possibly many other things present and produces nonsensical output. Targeting the logic in a better way again requires stripping out the whole system.

The newer system has no problems here as it connects the original format to the floats and does not even try to figure out any common formats for the whole expression: Screenshot from 2023-02-22 10-24-11

dlmsr commented 1 year ago

I am not really familiar with neither the old or the new input system. Do I understand correctly that the value of the Maxima variable stackfltfmt which drives stack_disp's float formatting is set by get_decimal_digits? If so, wouldn't it work to fix the function get_decimal_digits to use the AST instead of looping over the bytes of the cas string? I mean there's a MP_Float class which contains a member variable raw which could be used to set the formatting information. This way, the %e wouldn't be mistaken for scientific notation.

aharjula commented 1 year ago

Basically, yes, but the problem of the original issue is that we cannot have a singular format setting for the whole expression and should instead carry separate formats for all floats present in the expression. When doing that, we would also do the thing through the AST, and any misinterpretations of that %e would be skipped. Basically, the "new/future" system already knows how to do this, and we already have the tools, but those tools do not work with the old one.

So we do know how to fix this. However, the best way I can imagine is part of that "new" system, and as switching to that new system will also include some compatibility issues and various larger changes to the world, it is not happening now. In the proper fix we do not use a global formatting setting but instead go through the AST and identify the setting for each and every float, and that is being done by the filter 910, which then wraps those floats into special functions that ensure the correct display.

I do still hope that we can modify that get_decimal_digits to not consider that extra e as part of the float to fix this newest thing. The larger fix will, however, have to wait for the time when we can properly separate the value and display at the input system level, and that is the thing that will not happen now.

dlmsr commented 1 year ago

I do still hope that we can modify that get_decimal_digits to not consider that extra e as part of the float to fix this newest thing. The larger fix will, however, have to wait for the time when we can properly separate the value and display at the input system level, and that is the thing that will not happen now.

Yes, I was hoping for that. I am specifically interested in the issue with the %e since one of our questions has got this problem. I have seen there are regression tests for this function in tests/ast_container_test.php. Maybe it makes sense to add this issue with %e+4.3^k as a test case to keep track of the failure. WDYT?

aharjula commented 1 year ago

Indeed some test cases would need to be added here. As you can see from the existing ones, they have only focused on raw numbers with possible units and this feature was never really meant to function with general expressions.

I would imagine that all that needs to be done is to modify this if statement here: https://github.com/maths/moodle-qtype_stack/blob/081cd5eb89fb9f8ddc6b5c959e6c704543374fdf/stack/cas/ast.container.silent.class.php#L939-L941 I think that maybe it should just check if there have been any $meaningfuldigitsor $leadingzeros at all, and if not, then ignore the e.

Naturally, the same thing would need to be done for the CAS side version of this function here: https://github.com/maths/moodle-qtype_stack/blob/081cd5eb89fb9f8ddc6b5c959e6c704543374fdf/stack/maxima/utils.mac#L147-L149

In any case, hopefully, someone can do that and run the tests soonish.

aharjula commented 1 year ago

image

But there are still plenty of oddities remaining, for example, how x/-12.00 drops its extra decimals when this, ignores them: https://github.com/maths/moodle-qtype_stack/blob/e5a17ac2113f6588a0bb802ad8af5dfa84da6350/stack/cas/ast.container.silent.class.php#L915-L919 There is absolutely no need to pick anything out of the AST, and if someone does that, then it would be nice to check that the bit picked is actually a number.

dlmsr commented 1 year ago

Many thanks! It's a nice, quick fix without the major rewrite of get_decimal_places which I envisioned.

sangwinc commented 1 year ago

Matti, I think this commit https://github.com/maths/moodle-qtype_stack/commit/e5a17ac2113f6588a0bb802ad8af5dfa84da6350 has resulted in a new failing unit test https://github.com/maths/moodle-qtype_stack/actions/runs/4252294373/jobs/7395694579#step:17:95

This wasn't clear because of other parallel failing unit tests (which I've fixed!) Chris

aharjula commented 1 year ago

Chris,

Well, that is interesting. It did not fail on my machine. Now the bigger question is, what do we want that to give? Basically, 0 makes sense if we do simplification/evaluation of the expression and then force to integer presentation. But given that the display settings are primarily for the validation display, one would expect simplification to be off. Obviously, the last line of those three test cases I added is wrong, but again we need to decide what and with what simp we test. Maybe these test cases are now in the wrong place...

In any case, if you work over there, I would really like to see x/12.00 -> x/12.00 function correctly.

sangwinc commented 7 months ago

This is now fixed.