add tests for changes in tsutils

[kcpevey]

• add tests for changes in tsutils
• fix minor bugs that the tests exposed

I'd like @CommonClimate or @khider to review the expected values in the tests to ensure that things are working as expected.

[MarcoGorelli]

Nice, generally looks really good, just left a couple of minor comments

[MarcoGorelli]

why not check datum and exponent too?

[kcpevey]

Those are handled in the following tests (there is code following the time_name bit that modifies datum and exponent). This test set is trying to exclusively look at the time_name options and implications. There was no way to isolate it well, but its an attempt.

[CommonClimate]

@khider and I are confused about what this test does. Can you explain the goal?

[MarcoGorelli]

I think it's just to check what happens when some invalid time_unit is passed

still though, I'd suggest adding

        assert datum == 0
        assert exponent == 0

, it's only 2 lines of code, makes it consistent with the code above, and the logic inside time_unit_to_datum_exp_dir may well change someday such that time_name also affects datum and exponent

[kcpevey]

The goal of any test suite is to hit 100% coverage. That means we want the test suite to go down every pathway - every if/else, raise etc.

So this test in particular checks to make sure we get the appropriate warning when

1. time_unit is some unknown string, time_name is None
2. time_unit is some unknown string, time_name is some unknown string
3. time_unit is some unknown string, time_name is "age"

[MarcoGorelli]

I think it's just to check what happens when some invalid time_unit is passed

still though, I'd suggest adding

        assert datum == 0
        assert exponent == 0

, it's only 2 lines of code, makes it consistent with the code above, and the logic inside time_unit_to_datum_exp_dir may well change someday such that time_name also affects datum and exponent

[MarcoGorelli]

Looks good to me, over to USC

[CommonClimate]

This is stretching my Python knowledge a bit (I'd never heard of frozen sets), but it generally looks good. However, I decided to streamline time_unit_to_datum_exp_dir() because we were overdoing things. I'm reasonably confident that this version catches all sensible unit specifications (famous last words...) and it removes a couple of tests, which you can just comment out. Can you look at my latest commit and make the changes that appear necessary?

BTW, @khider pointed out that those conversion functions should be in tsbase.py, not tsutils.py, see #299. When do you think it safest to move them over?

[CommonClimate]

Specifically, I removed the if/then statement having to do with match_a, because we set exponent = 0 by default, so that covers that case. Also, the case of "years BP" is already covered by elif any(c in tu for c in ['bp', 'bnf', 'b1950']), so I removed it as well.

[CommonClimate]

Sorry if the back&forth makes things difficult for your test-writing ; I don't always see the whole picture when I'm writing the code, but this gave me the opportunity to think about it more, hence the changes.

[kcpevey]

I removed the if/then statement having to do with match_a, because we set exponent = 0 by default

The way its currently written (can't remember who coded this), it will warn if it doesn't match the input time_unit and then default to "years CE". Is that ok?

Also, the case of "years BP" is already covered by elif any(c in tu for c in ['bp', 'bnf', 'b1950']), so I removed it as well.

So "years BP" must be input as time_unit of one of these: ['bp', 'bnf', 'b1950']. These values ['yr BP', 'yrs BP', 'years BP'] will no longer be allowed to specify as time_unit. Is that ok?

[CommonClimate]

The way its currently written (can't remember who coded this), it will warn if it doesn't match the input time_unit and then default to "years CE". Is that ok?
Yes, this default makes sense for a lot of the examples we currently have.

So "years BP" must be input as time_unit of one of these: ['bp', 'bnf', 'b1950']. These values ['yr BP', 'yrs BP', 'years BP'] will no longer be allowed to specify as time_unit. Is that ok?

The way I wrote it, ['yr BP', 'yrs BP', 'years BP'] will be caught by the clause elif any(c in tu for c in ['bp', 'bnf', 'b1950']), and assign the right datum and direction. Since exponent is set to 0 by default, we should be all good, no?

[CommonClimate]

Hi @kcpevey , I starting debugging the merge from 'new_objects' into Development and found a few bugs in the way I cast the logical net over "time_units". My latest commit (in a new branch, see a1fa119) makes it explicit that the time_unit can contain compound statements like "ky BP", so the code now parses the first statement (about exponent) and then the second (BP, which is about datum and direction). It's now passing all the pytests, some of which I modified to throw a few curve balls. Please have a look at your leisure.