Add backwards compatible collect function

[rdoyle45]

This pull request adds a new function, collect(), to xBOUT which aims to replicate the behaviour of https://github.com/boutproject/boutdata/blob/master/boutdata/collect.py.

Feature suggested in #9

[pep8speaks]

Hello @rdoyle45! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-12-05 16:11:04 UTC

[TomNicholas]

This is great, thanks @rdoyle45 .

Main thing it needs is tests to check the indexing behaving as expected. Then this can also be tested against the actual old collect function.

[TomNicholas]

No, isel accepts a dictionary where the values are either an int, a list of ints, or a slice. The only difference we have to account for is that isel treats a list of indexes as a list individual locations you want to select, so [5,10] would give you the 5th and 10th element, not elements 5 through 10 as the old collect does.

…

On Mon, 28 Oct 2019, 13:36 Rhys Doyle, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In xbout/load.py <#59 (comment)>: > + selection = {} + + for i in range(len(dims)): + + if indexers[i] != None: + + if isinstance(indexers[i], int): + selection[dims[i]] = [indexers[i]] + else: + selection[dims[i]] = indexers[i] Very true, I will simplify now. For specific index extraction I think iselneeds to given a list object so isinstance(ind, int) would need to be checked too? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#59?email_source=notifications&email_token=AISNPI463U2D7YWTJDCPYITQQ3TGPA5CNFSM4JFZVJV2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJMWVXI#discussion_r339564916>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AISNPI7GNFTW364BRCNMACDQQ3TGPANCNFSM4JFZVJVQ> .

[rdoyle45]

Apologies I meant say within a dictionary.

When I was testing isel with for example dict = {'x' : 3} ds.isel(dict) would remove the x dimension entirely. When I use dict = {'x' : [3]} a single x index is extracted.

Am I missing something or is this correct?

[TomNicholas]

Hmm I'm actually not sure, I was testing just now by doing `isel(t=5)`, but perhaps I should have been doing `isel(dict('t' : 5))`. What I suggest is that you practice test-driven-development here. Write out small tests for each case that should work, including handling `None` and so on. Then change the collect logic until the tests all pass. That process will be less likely to leave errors than trying to guess how `isel` behaves and hoping you've covered each possibility.

…

On Mon, 28 Oct 2019, 13:55 Rhys Doyle, ***@***.***> wrote: Apologies I meant say within a dictionary. When I was testing isel with for example dict = {'x' : 3} ds.isel(dict) would remove the x dimension entirely. When I use dict = {'x' : [3]} a single x index is extracted. Am I missing something or is this correct? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#59?email_source=notifications&email_token=AISNPIZ7VDHBWPJS5KLIZZTQQ3VMHA5CNFSM4JFZVJV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECM6OVI#issuecomment-546957141>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AISNPI43JDXRU2XSI7BYOGTQQ3VMHANCNFSM4JFZVJVQ> .

[rdoyle45]

Thanks Tom, will do. I'll check that now.

Hmm I'm actually not sure, I was testing just now by doing isel(t=5), but perhaps I should have been doing isel(dict('t' : 5)). What I suggest is that you practice test-driven-development here. Write out small tests for each case that should work, including handling None and so on. Then change the collect logic until the tests all pass. That process will be less likely to leave errors than trying to guess how isel behaves and hoping you've covered each possibility.
…
On Mon, 28 Oct 2019, 13:55 Rhys Doyle, @.***> wrote: Apologies I meant say within a dictionary. When I was testing isel with for example dict = {'x' : 3} ds.isel(dict) would remove the x dimension entirely. When I use dict = {'x' : [3]} a single x index is extracted. Am I missing something or is this correct? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#59?email_source=notifications&email_token=AISNPIZ7VDHBWPJS5KLIZZTQQ3VMHA5CNFSM4JFZVJV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECM6OVI#issuecomment-546957141>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AISNPI43JDXRU2XSI7BYOGTQQ3VMHANCNFSM4JFZVJVQ .

[rdoyle45]

Will wait until #55 is merged

[codecov-io]

Codecov Report

Merging #59 into master will decrease coverage by 23.95%.
The diff coverage is 76.66%.

@@             Coverage Diff             @@
##           master      #59       +/-   ##
===========================================
- Coverage   68.69%   44.74%   -23.96%     
===========================================
  Files           7        8        +1     
  Lines         444      789      +345     
  Branches       95      152       +57     
===========================================
+ Hits          305      353       +48     
- Misses         85      380      +295     
- Partials       54       56        +2

Impacted Files	Coverage Δ
xbout/load.py	77.41% <76.66%> (-0.13%)	⬇️
xbout/plotting/utils.py	4.91% <0%> (-15.09%)	⬇️
xbout/plotting/animate.py	54.87% <0%> (-8.76%)	⬇️
xbout/boutdataarray.py	71.42% <0%> (-3.58%)	⬇️
xbout/plotting/plotfuncs.py	9.33% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e53c897...18db7f0. Read the comment docs.

[rdoyle45]

I have added tests to check each dimension and index type. I have kept the list indexing as exclusive when using [start, end, step] as it is in boutdata at present.

This may change depending on the results of boutproject/BOUT-dev#1861 , and therefore we should wait to merge. However all tests should pass at present and any change in #1861 should not make the tests fail.

What do you think @johnomotani? I think the tests are quite robust now.

[johnomotani]

Looks good to me. Probably worth holding off on merging for a little while to see if there's a resolution to the discussion in boutproject/BOUT-dev#1861.

[rdoyle45]

Looks good to me. Probably worth holding off on merging for a little while to see if there's a resolution to the discussion in boutproject/BOUT-dev#1861.

Agreed

[johnomotani]

Following discussion on boutproject/BOUT-dev#1862, I think we can remove support and tests for the 3-element list form.

[rdoyle45]

Following discussion on boutproject/BOUT-dev#1862, I think we can remove support and tests for the 3-element list form.

Removed. I think this is good to go? @TomNicholas @johnomotani

Issue has been resolved - indexing now tested against old collect.