Change check in fonll

[evagroenendijk]

I changed this check in fonll because for me the XIA is not necessarily set in all theory cards but it is in the fonll cards. I don't know if this is the best solution, so please let me know!

[scarlehoff]

I personally think we should be checking a subset of keys instead of removing keys and checking the rest?

My two cents, instead of the current code at worst I think we should have

        dont_check = ["FNS", "PTO", "PTODIS", "NfFF", "ID", "FONLLParts", "Comments" "XIA"]

        differ = []
        for key in theorycards[0].keys():
            if key in dont_check:
                continue
            if len({card.get(key)) != 1:
                differ.append(key)

        if differ:
            raise ValueError(f"The following keys differ between different FONLL theory cards: {differ}")

That way we also get the differences instead of the current "though luck"...

But I'm ok with merging the current change and calling it a day.

Pinging also @felixhekhorn as he might have a strong opinion here.

[felixhekhorn]

I agree with the change suggested by @scarlehoff . Just fixing some typo and return value

        dont_check = ["FNS", "PTO", "PTODIS", "NfFF", "ID", "FONLLParts", "Comments" "XIA"]

        differ = []
        for key in theorycards[0].keys():
            if key in dont_check:
                continue
            if len({card.get(key) for card in theorycards}) != 1:
                differ.append(key)
            del theorycards[0][key]

        if len(differ) > 0:
            raise ValueError(f"The following keys differ between different FONLL theory cards: {differ}")

(still coding in the void so care should be taken

[felixhekhorn]

I guess you tested by hand? Adding a unit test would be really great (as this is obviously a non-trivial business and even caused trouble in real life), else looks okay

[felixhekhorn]

please fix pre-commit

[evagroenendijk]

I guess you tested by hand? Adding a unit test would be really great (as this is obviously a non-trivial business and even caused trouble in real life), else looks okay

Okay, so you mean a test in tests/test_fonll.py then right? And the test would be to take a dummy fonll fk table and see if it matches the theory card?

[felixhekhorn]

I guess you tested by hand? Adding a unit test would be really great (as this is obviously a non-trivial business and even caused trouble in real life), else looks okay

Okay, so you mean a test in tests/test_fonll.py then right? And the test would be to take a dummy fonll fk table and see if it matches the theory card?

Mmm, almost. The thing we are looking at here is that several FK table match with each other - but else yes, we would fake some FK tables. If you want to go for it, maybe we can directly use the pineko functions to generate these theory cards in the first place (i.e. check that pineko is self-consistent).

[evagroenendijk]

I guess you tested by hand? Adding a unit test would be really great (as this is obviously a non-trivial business and even caused trouble in real life), else looks okay

Okay, so you mean a test in tests/test_fonll.py then right? And the test would be to take a dummy fonll fk table and see if it matches the theory card?

Mmm, almost. The thing we are looking at here is that several FK table match with each other - but else yes, we would fake some FK tables. If you want to go for it, maybe we can directly use the pineko functions to generate these theory cards in the first place (i.e. check that pineko is self-consistent).

Okay, perfect. I can try to do this at some point (I think it would be good because I already found a little mistake in my implementation ;))