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

Don't overwrite default numpy bindings from liglow in record.io #4525

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

cdcapano
Copy link
Contributor

Currently, if you import the record module in pycbc.io, it will cause numpy to start casting float and int to float32 and int32 respectively. The default in numpy is to cast float and int to 64 bit scalars. The cause of this is due to the numpy scTypeDict being updated with ligolw bindings in record.py. In ligolw, float is cast to float32 and int to int32. This patch fixes that by not including the float and int mappings from ligolw. This will cause single precision values to be cast to double when reading a ligolw xml file, but that should only result in some meaningless extra digits being added to values.

@cdcapano
Copy link
Contributor Author

Changed to not include any bindings that numpy doesn't already define (so, adds double to the list of exclusions).

Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Only stylistic comment is why to use variable names preceded by an underscore for the internal variables in the dict-comprehension?

@cdcapano
Copy link
Contributor Author

@spxiwh I was thinking these variables might be left over in the io.record namespace, so preceded them with _ to make them hidden. But you're right, because they're in a dict comprehension they don't survive outside of it, so that wasn't necessary. It doesn't hurt though, and doesn't violate PEP8, so I'm just going to leave it as is.

@cdcapano cdcapano merged commit cee6437 into gwastro:master Oct 11, 2023
31 checks passed
@cdcapano cdcapano deleted the ligolw_dtype_fix branch October 11, 2023 20:39
PRAVEEN-mnl pushed a commit to PRAVEEN-mnl/pycbc that referenced this pull request Nov 3, 2023
…tro#4525)

* Don't overwrite default numpy bindings from liglow in record.io

* Change to not include any bindings that numpy already defines
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
…tro#4525)

* Don't overwrite default numpy bindings from liglow in record.io

* Change to not include any bindings that numpy already defines
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
…tro#4525)

* Don't overwrite default numpy bindings from liglow in record.io

* Change to not include any bindings that numpy already defines
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
…tro#4525)

* Don't overwrite default numpy bindings from liglow in record.io

* Change to not include any bindings that numpy already defines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants