Consider capitalization

[hwalinga]

In case of ambiguous matches, this will re-attempt the algo by swapping the case of the last letter of the unit, and see if this creates a better match. It will also throw a warning if an unambiguous match cannot be found.

EDIT: Fixes #161

[hwalinga]

Oh, I based of master instead of dev. Should I do this again?

EDIT: I rebased.

[coveralls]

Pull Request Test Coverage Report for Build 741

• 58 of 70 (82.86%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.8%) to 96.708%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
quantulum3/no_classifier.py	8	9	88.89%
quantulum3/disambiguate.py	33	44	75.0%
<!--	Total:	58	70

Totals
Change from base Build 734:	-0.8%
Covered Lines:	1263
Relevant Lines:	1306

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 655

• 29 of 32 (90.63%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 96.984%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
quantulum3/classifier.py	29	32	90.63%
<!--	Total:	29	32

Totals
Change from base Build 654:	-0.2%
Covered Lines:	1222
Relevant Lines:	1260

---

💛 - Coveralls

[nielstron]

The changes look good! However I feel that this procedure should be available to non-classifier disambiguation as well and hence lie a layer above the current changes.

Further I would prefer to have systematic changes (i.e. swapping all letters or searching for the unit with least differences in capitalization) rather than swapping only the last letter which seems quite random and inconsistent to me (what about the unit "Km", we should swap the first letter here for a perfect match. What about "litre", is it useful to check for "litrE"?)

[nielstron]

Please keep changes unrelated to the specific issue out of this pull request. Seperate codestyle improving pull requests will be warmly welcomed

[hwalinga]

OK,

I created #164 for all my import sorting and there was a block not black formatted.

I have moved the logic one layer up and also implemented it for no classifier.

There is no need to consider any other case than the last letter (unit letter) to swap the case of, because it is only the first letter that causes the ambiguity because of the ambiguity in casing of the SI-prefixes. (milli and Mega, and pico and Peta)

I did check if the unit was longer than 2 letters and disabled that additional case swapping for those.

[nielstron]

There is no need to consider any other case than the last letter (unit letter) to swap the case of, because it is only the first letter that causes the ambiguity because of the ambiguity in casing of the SI-prefixes. (milli and Mega, and pico and Peta)

If you really only want to apply this check to prefixed units, we should check whether the unit has any SI-prefix and only apply this workaround in that case.

On the other hand, the two letter check is too specific, consider for example the unit Bar. If someone writes "1 MBAR" it is currently either a milli- or a Mega-Bar, but according to this change it should be considered a Megabar preferably (on my device, rather deterministically, millibar is returned)

[hwalinga]

Good suggestions. I applied them.

[hwalinga]

Hmmm.. There is no Mbar in units.json, only a mbar and a μbar. So, it won't work anyway, but at least the code for it is now in place.

[nielstron]

There is no Mbar in units.json

I think this should be fixed then since the goal would be to support all Wikipedia featured units :sweat_smile:

To fix the reduction in coverage, you would need to add a test case that triggers the branch you just added.

[hwalinga]

Your test suite is a bit weird. Automatically loading a bunch of test cases is clever, but you are better off designing the tests yourself so that you can have tests that are specific to certain edge cases. Or at least have those in addition to the automated tests. Now for example when one loaded test fails, all are flagged as failed. When a test fails you want to know what the specific problem is.

In addition to that as a tip, if you load multiple cases and test in a loop, at least use subtests:

https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests

Anyway, without any proper example on how to write a test for an edge case from your side it is very hard for me to entangle your code to write a test case that works. I attempted one, but I cannot create a Unit that is equal somehow. Can you take a look what I am doing wrong?

[nielstron]

I see your point about the test suite. Thanks for pointing me to the subtest system. I will have a look into it. Related to #157 there will likely be also a class of test cases that merely checks whether an error is thrown or not but ignores the outcome.

Regarding your issue, I will have a look soon but am currently rather busy. Until now I've had trouble to find the cause of test failures myself.

[hwalinga]

OK, ping me when you updated the test suite, than I can see how I can learn from that to make tests for this PR.

[nielstron]

I enhanced the test suite, feel free to have a look at the changes of #171

[hwalinga]

The test suite still misses a simple single test like I would want to add as well. It still all obscure automated testing, now with subtests though.

To repeat what I said before, a proper test suite would test specific different cases. It is not good enough to just go grab a random bunch of cases and test them all and if you succeed more than 80 % call it good enough.

With this PR I want to test specific cases, but without an example I cannot get it going so easily. I tried, if you look at this PR I have a test, but it fails, and since I don't have an example to copy from I don't know what I am doing wrong. I will paste it here in the comment as well for you to see:

So what is wrong here?

class ClassifierTestEdgeCases(unittest.TestCase):
    """Test suite that tests certain specifically designed edge cases to confuse the classifier."""

    def test_wrong_capitalization(self):
        parsed_unit = p.parse("1nw")
        unit = Quantity(
            value=1,
            unit=Unit(
                name="nanowatt",
                entity=Entity("power"),
                dimensions=[{"base": "nanowatt", "power": 1, "surface": "nW"}],
            ),
        )
        self.assertEqual(parsed_unit, unit)

PS. The steps under contributing are not complete, because the test suite requires some more requirements.

[nielstron]

Note that there are two distinct classes of test cases. One class contains ambiguous cases, one class contains cases that (given the rules provided) should always output a certain output. For the first class, only a percentage of classes is required to be recognized correctly. For the second class the output has to match correctly.

Your change affects the classifier (ambiguous cases) but it is possible to create cases where the output is deterministic. Thereforce the test case should be added to the latter class. It is simply added by extending "tests/quantities.json" with a sample sentence (in your case for example "The energy in this battery is 1nW") and the expected units parsed (see the other examples in the file). I hope that helps.

[hwalinga]

Yes, but the problem of that approach is that those tests are not isolated. (1) I cannot run a single test that gives problems. (2) I have to run the whole suite every time. (3) I don't see what specific test case is testing.

It is not a bad way to run your tests like that, but having specific designed tests that test a specific feature of the code would help a lot. Here for example I specifically wrote a test that does test_wrong_capitalization. If I put that in that json all the context and isolation is gone.

So, how can I run a single test like the way I showed in my previous comment.

(Also if you stick to the default way of writing tests people can add their own tests. It is now unclear how your test suite works.)

[nielstron]

The way I personally handle this is to create a file (i.e. "test.py") and write in it the desired test case behavior (i.e. parser.parse("...") == [...]). A number of IDEs support running this in debug mode as does pdb

Then if everything works as expected I add the required test to the json file.

I agree that this is not optimal procedure. However with the sheer amount of test cases and possible interactions of rules it seems to me that the checking cannot be nailed down to a specific rule and has to be considered holistically.

[hwalinga]

OK, well I don't think that the best way to do it, but I think I already made my point. It would be a good idea to make some sort of contributing doc in which you explain how somebody contributing can add a test case.

I have added one test case. The other cases discussed here cannot be added since units.json misses things like MegaBar and MegaLitre.

[hwalinga]

I am not so sure what is going on with "1 '2". What triggers that to be parsed? Anyway, I wrote a work around.

Also, just a tip, don't use try: ... except Exception as e: self.fail(...) in a test case. You lose the stacktrace during debugging. (pylint does not flag that as wrong for nothing.)

[hwalinga]

@nielstron Would you still be able to take a look at this?