Update All SI Units #87

tkuschel · 2023-04-06T02:20:31Z

I have been annoyed for a very long time that you can't just use all the units with the SI prefixes. For the electronics lectures at the university of applied sciences I asked students to use the scientific notation for the time being. Then I moved on to using conversion formulas. Long story short:
I implemented a third choice at the "Unit Settings "+"Basic conversion rules" called "All SI units". With this setting, for any unit, the possibility to enter all SI prefixes is given.
Example: Unit "µA" given. As a result from a question you have to enter e.g. "22 µA".
Now, with this "All SI units" - setting, all these answers are correct:

22 µA
0.022 mA
22e-6 A
22000 pA
22 uA

Additionally this functionality works with any unit e.g. kΩ, VA, kmol, mol, cd, ...

new feature: possiblity to enter µ (mu, and greek letter) in addition to u
new feature: conversion rules: add fullsiunit beside of commonsiunit and none.
removed: not used function get_unit_mapping(), and get_dimension_list()

PhilippImhof · 2023-04-06T08:14:05Z

Thank you very much for this nice contribution. I will have a closer look at it soon. You chose a good time, because I am currently rewriting the whole parsing/evaluation engine and was thinking about ways to improve the unit stuff.

tkuschel · 2023-04-06T20:08:52Z

If there are more then one Units given in the field "Unit" e.g.
"mΩ = mohm = mm^2 kg s^-3 A^-2"

This did not work any more. Also I think the "mm^2 kg s^-3 A^-2" did not work in the past, that could be also implemented to use prefixes for such a unit.
Several testcases at http://159.69.109.101/course/view.php?id=2``

It looks like that a refactoring of the whole prefix calculation would be fine. For instance the following feature would be great:

prefix handling with m^2 or m^3
very complex handling of this->mapping and additonal_rules; could be more flexible and easier data structures

I updated my solution, still room for improvements...

Thomas

PhilippImhof · 2023-04-07T07:56:07Z

Thanks! The whole unit thing suffers from a huge lack of unit and, to a lesser extent, acceptance testing. I think now is a good moment to add some tests. Don't worry, I will try to find some time to write them :)

I also agree with your remarks about prefix calculation. That code is very old and comes with some limitations, especially w.r.t. maintainability. My ultimate goal is to rewrite that part, but that can only be done once the new parsing/evaluation engine is ready.

PhilippImhof

Thanks again for your contribution.

I have finally found some time to look at the code and made a few remarks. I have not yet tested the functionality, because I would like to prepare some automated testing for the unit conversion (as it is now) first.

PhilippImhof · 2023-04-12T08:12:34Z

conversion_rules.php

-F: m u n p f;
-T: k m u n p;
-H: k m u n p;
+Ω: u µ μ m k M G T P;


This is (U+03A9 / GREEK CAPITAL LETTER OMEGA). I suggest adding Ω (U+2126 / OHM SIGN) as well.

PhilippImhof · 2023-04-12T08:16:27Z

CHANGES.md

Please include the changelog entries in the PR, so they can be included upon preparation of the next release, together with the correct version number and release date.

PhilippImhof · 2023-04-12T08:21:13Z

answer_unit.php

+        'E' => 1e18, 'Z' => 1e21, 'Y' => 1e24, 'f' => 1e-15, 'a' => 1e-18, 'z' => 1e-21, 'y' => 1e-24,
+        'R' => 1e27, 'Q' => 1e30, 'r' => 1e-27, 'q' => 1e-30 );
+    // For convenience, u can be used for micro-, rather than "mu", which has multiple similar UTF representations.
+    // Note: All UTF representations of µ can be used too.


Suggested change

// Note: All UTF representations of µ can be used too.

// Note: One can use GREEK SMALL LETTER MU (U+03BC) or MICRO SIGN (U+00B5).

PhilippImhof · 2023-04-12T08:23:48Z

answer_unit.php

+     */
+    public function parse_prefix_unit($unit) {
+        if (is_array($unit)) {
+            throw new Exception('parse_prefix_unit does not handle arrays!');


The string should be in lang/en/qtype_formulas.php and imported with get_string.

PhilippImhof · 2023-04-12T08:24:05Z

answer_unit.php

+    public function parse_prefix_unit($unit) {
+        if (is_array($unit)) {
+            throw new Exception('parse_prefix_unit does not handle arrays!');
+            return '';


Suggested change

return '';

Unreachable code.

PhilippImhof · 2023-04-12T08:28:18Z

answer_unit.php

+        foreach (static::$prefix_scale_factors as $i => $prefix) {
+            if (str_starts_with($unit,$i)) {
+                $unit = substr($unit, strlen($i));
+                break;


The break is not needed IMHO.

Suggested change

break;

PhilippImhof · 2023-04-12T08:29:45Z

answer_unit.php

-                    if (preg_match('/['.self::$unit_exclude_symbols.']+/', $unit_name)) {
-                        throw new Exception('"'.$unit_name.'" unit contains unaccepted character.');
+                    if (preg_match('/['.self::$rule_exclude_symbols.']+/', $unit_name)) {
+                        throw new Exception('"'.$unit_name.'" rule contains unaccepted character.');


The string should be in lang/en/qtype_formulas.php and imported with get_string. (I know it wasn't done like this before, but if we're at it...)

PhilippImhof · 2023-04-12T08:38:16Z

answer_unit.php

-    public function assign_default_rules($default_id, $default_rules) {
-        if ($this->default_id == $default_id) {
+    public function assign_default_rules($default_id, $default_rules, $part_unit = '') {
+        if (($default_id == 2) && ($part_unit != $this->part_unit)) {


I think it would probably be better to have a class constant ALL_SI_UNITS = 2 and use it here. This would make the code easier to understand in a few years.

PhilippImhof · 2023-04-12T08:42:40Z

conversion_rules.php

 ');

+        $this->basicunitconversionrule[2] = array(get_string('allsiunits', 'qtype_formulas'), '');


I am not sure I am getting this right: You changed the conversion rules for commonsiunit, but you define allsiunits anyway. But don't the changes bring the desired functionality to commonsiunit as well? Would there be any reason for a user to stick with commonsiunit versus allsiunits?

PhilippImhof · 2023-04-12T08:44:00Z

version.php

Please do not change the file version.php for the moment. It will be updated during the preparation of the release, in order to avoid merge conflicts among concurrent PRs.

PhilippImhof · 2023-04-12T12:28:44Z

I have added a very basic test for the "common SI unit" conversion rules and took the liberty to merge this into your PR. We will be able to extend that test for the stuff you added.

Update All SI Units

2f716d6

PhilippImhof self-assigned this Apr 6, 2023

PhilippImhof added the enhancement label Apr 6, 2023

FIX several units given with connected =

e586ce8

PhilippImhof requested changes Apr 12, 2023

View reviewed changes

PhilippImhof mentioned this pull request Apr 12, 2023

add basic test for unit conversion #88

Merged

Merge branch 'master' into pr/87

0911283

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update All SI Units #87

Update All SI Units #87

tkuschel commented Apr 6, 2023

PhilippImhof commented Apr 6, 2023

tkuschel commented Apr 6, 2023 •

edited

Loading

PhilippImhof commented Apr 7, 2023

PhilippImhof left a comment

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof Apr 12, 2023

PhilippImhof commented Apr 12, 2023

	// Note: All UTF representations of µ can be used too.
	// Note: One can use GREEK SMALL LETTER MU (U+03BC) or MICRO SIGN (U+00B5).

		');

		$this->basicunitconversionrule[2] = array(get_string('allsiunits', 'qtype_formulas'), '');

Update All SI Units #87

Are you sure you want to change the base?

Update All SI Units #87

Conversation

tkuschel commented Apr 6, 2023

PhilippImhof commented Apr 6, 2023

tkuschel commented Apr 6, 2023 • edited Loading

PhilippImhof commented Apr 7, 2023

PhilippImhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippImhof commented Apr 12, 2023

tkuschel commented Apr 6, 2023 •

edited

Loading