__len__ for wrappers in dataset.py

[Mizzcoollizz]

No description provided.

[codecov]

Codecov Report

Merging #1913 (671822d) into master (cc17661) will decrease coverage by 1.02%.
The diff coverage is 100.00%.

❗ Current head 671822d differs from pull request most recent head e55a9ce. Consider uploading reports for the commit e55a9ce to get more accurate results

@@            Coverage Diff             @@
##           master    #1913      +/-   ##
==========================================
- Coverage   88.46%   87.43%   -1.03%     
==========================================
  Files          17       17              
  Lines        2306     2213      -93     
==========================================
- Hits         2040     1935     -105     
- Misses        266      278      +12     

Impacted Files	Coverage Δ
h5py/_hl/dataset.py	92.37% <100.00%> (-0.15%)	⬇️
h5py/_hl/filters.py	85.12% <0.00%> (-7.73%)	⬇️
h5py/_hl/dims.py	95.18% <0.00%> (-0.70%)	⬇️
h5py/_hl/datatype.py	95.00% <0.00%> (-0.66%)	⬇️
h5py/_hl/selections.py	87.43% <0.00%> (-0.57%)	⬇️
h5py/_hl/base.py	95.83% <0.00%> (-0.24%)	⬇️
h5py/_hl/attrs.py	96.74% <0.00%> (-0.16%)	⬇️
h5py/_hl/group.py	96.66% <0.00%> (-0.13%)	⬇️
h5py/_hl/vds.py	96.52% <0.00%> (-0.03%)	⬇️
... and 1 more

---

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 cc17661...e55a9ce. Read the comment docs.

[takluyver]

Bedankt!

Could I ask you to add a couple of little tests for these as well? Search for e.g. test_asstr to find relevant existing tests.

[takluyver]

The class names aren't really obvious in the public API - I'd probably describe them as dset.astype() etc.

[tacaswell]

Thank you for working on this @Mizzcoollizz !

The failure is still in .authors.yml,

'elize.herrewijnen@gmail.com' does not have 'Elize' listed! Please add the name to 'elize.herrewijnen@gmail.com' in '.authors.yml'. For example:

- name: Elize
  email: elize.herrewijnen@gmail.com

Or add an alias:

- name: Some Body Else
  email: elize.herrewijnen@gmail.com
  aliases:
    - Elize
All CLI and import requirements met for authors activity
ERROR: InvocationError for command /home/vsts/work/1/s/.tox/rever/bin/rever check --activities authors,version_bump,changelog (exited with code 1)

It looks like it is failing due to a miss-match between the name you put in Authors (your full name) and the name you told git and is being parsed out of the commit (just your first name).

---

Could you please add a test for this? I know it seems trivial, but it will make sure we do not accidentally delete or refactor the functionality away in the future!

Are there any other things would would want to forward from dset? Maybe {shape, ndims, dims, chunks} which are in the same category as len? Would we also want to forward a majority of the API? Not sure how far down that path we want to go (either now or ever).

[tacaswell]

Sorry you got simultaneous reviews!

[takluyver]

@Mizzcoollizz just checking if you're planning to come back to this? I think tests were the main thing both @tacaswell and I asked for in our reviews. 🙂

[takluyver]

Thanks! Can you also add a test for FieldsWrapper? You can find a relevant test to start from here:

h5py/h5py/tests/test_dataset.py

Lines 1255 to 1275 in b578203

	def test_fields(self):
	dt = np.dtype([
	('x', np.float64),
	('y', np.float64),
	('z', np.float64),
	])
	testdata = np.ndarray((16,), dtype=dt)
	for key in dt.fields:
	testdata[key] = np.random.random((16,)) * 100
	self.f['test'] = testdata
	# Extract multiple fields
	np.testing.assert_array_equal(
	self.f['test'].fields(['x', 'y'])[:], testdata[['x', 'y']]
	)
	# Extract single field
	np.testing.assert_array_equal(
	self.f['test'].fields('x')[:], testdata['x']
	)

Also, unfortunately the author check is now seeing two different email addresses, and it wants to find both in .authors.yml.

[takluyver]

I added a test for len(dset.fields()). Unfortunately, the CI is still failing on the author information. It's getting confused because your commits have two different names (with & without your surname) and two different emails (gmail & kpn).

If you're happy for all of those details to be recorded in Github, the easiest thing is just add them all to .authors.yml, using aliases for names and alternate_emails for extra emails. If you'd rather not record some of them, we'll need to rewrite some commits, which is possible but a bit fiddly. If you want that but aren't sure how to do it, let me know and I can squash your commits together into a single one with whichever name & email you prefer.

[takluyver]

Alternatively, I'm proposing we get rid of this authors check entirely (#1940). If the other maintainers agree with me, that will solve the issue.

[takluyver]

OK, we've got rid of the authors check that was causing these problems.

As things stand, your two email addresses (gmail & kpn) will still be recorded in the commit history (git log). If you'd prefer not to have one of these recorded, let me know soon and I'll clean it up. Otherwise, I'll assume this is OK.

[Mizzcoollizz]

OK, we've got rid of the authors check that was causing these problems.

As things stand, your two email addresses (gmail & kpn) will still be recorded in the commit history (git log). If you'd prefer not to have one of these recorded, let me know soon and I'll clean it up. Otherwise, I'll assume this is OK.

Hi! Thank you! Could you remove the kpn email? That email address doesn't exist and I'm not sure where it came from...

[takluyver]

No problem.

[takluyver]

Thanks for your patience 🙂