Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the c-side constructor of numpy's dtype to expose the typobj attibute #1439

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kif
Copy link
Contributor

@kif kif commented Nov 25, 2019

close #1436

@s-sajid-ali could you test this branch ?

@kif kif requested a review from takluyver November 25, 2019 08:49
@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #1439 into master will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
- Coverage   84.94%   84.82%   -0.12%     
==========================================
  Files          17       17              
  Lines        2105     2128      +23     
==========================================
+ Hits         1788     1805      +17     
- Misses        317      323       +6
Impacted Files Coverage Δ
h5py/__init__.py 60.27% <0%> (+6.27%) ⬆️

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 273dfa9...8f253d7. Read the comment docs.

@takluyver
Copy link
Member

Close/reopen to rerun tests.

@takluyver takluyver closed this Nov 25, 2019
@takluyver takluyver reopened this Nov 25, 2019
@takluyver
Copy link
Member

I can't reproduce the error either. :-/

@takluyver takluyver added this to the 3.0 milestone Nov 25, 2019
@takluyver
Copy link
Member

Apparently @s-sajid-ali can't reproduce this either. @kif do you think the change is an improvement anyway, or should we close it?

@scopatz
Copy link
Member

scopatz commented Nov 27, 2019

This change is probably an improvement as it uses the CAPI

@kif
Copy link
Contributor Author

kif commented Nov 28, 2019

Using the C-api is not always a good move. In this case, apparently it changes little. I am pretty agnostic. if @s-sajid-ali cannot reproduce the bug, I would probably drop the PR as it solves no issue.

@kif kif added the more-info label Nov 29, 2019
@kif
Copy link
Contributor Author

kif commented Nov 29, 2019

We can leave it open for a month or so in case this bug pops-out again then.

@takluyver
Copy link
Member

I'm inclined to agree with 'if it ain't broke, don't fix it'. Good idea to leave it open and obvious for a bit in case someone else can reproduce it, though.

@scopatz
Copy link
Member

scopatz commented Dec 2, 2019

This still seems like a positive change to me

@takluyver takluyver removed this from the 3.0 milestone Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AttributeError: 'numpy.dtype' object has no attribute 'typeobj'
3 participants