-
Notifications
You must be signed in to change notification settings - Fork 22
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
Stop casting ints to floats #964
Conversation
…teger valued columns to floats
for more information, see https://pre-commit.ci
This breaks backwards compatbility at the level of from spikeinterface.core import generate_sorting
from pynwb.testing.mock.file import mock_NWBFile
nwbfile = mock_NWBFile()
sorting1 = generate_sorting(num_units=4)
sorting1 = sorting1.rename_units(new_unit_ids=["a", "b", "c", "d"])
sorting2 = generate_sorting(num_units=4)
sorting2 = sorting2.rename_units(new_unit_ids=["c", "d", "e", "f"])
sorting2.set_property(key="an_integer_property", values=[1, 2, 3, 4])
from neuroconv.tools.spikeinterface import add_sorting
from pynwb.testing.mock.file import mock_NWBFile
add_sorting(sorting=sorting1, nwbfile=nwbfile)
add_sorting(sorting=sorting2, nwbfile=nwbfile) I am kind of surprise that we don't have a test for this. |
More comments:
|
@h-mayorquin thanks for looking into this. As I asked here, how often does this use-case really come up where we'd have units that have different properties in the same Units table? What if we catch this edge case, and recast to floats only when we need to and throw a warning indicating that we are doing this? |
I think a more natural use-case for expanding a table would be to add rows that have columns that are consistent with the existing table. That is doable without casting to floats, so I think |
The example I mentioned before was working on consensus algorithms: adding sorting results from different sorters with different properties (one of which is type int).
Yes, I thought about it: the table is expandable in many cases that don't require the casting. What do you think of One thing that is not clear how to handle is compatbility across calls of A consideration that just made me change my mind to support the default that you want is that the current approach is loosy for int64. int32 can be safely cast to float 64 (the mantissa of the later has enough bytes) by doubling the memory cost but if the type is int64 we will be loosing information in most systems (asuming hdmf has the types of numpy). Anyway, we don't have the logic to handle any of those considerations so we are currently doing loosy conversions for int64. In other words the current behavior has a bug. I really wish that we had a nullable value for int types but is unlikely that hdf would support that |
Yes, as I wrote above I changed my mind about the default. Given that we can guarantee lossless casting for in64 it is probably better to re-think the whole thing. So my current position is: Let's not cast by default and produce error friendly messages when the need to have nullable values of int arises which is:
I probably think that focusing on the missing values and what to do with them boht as a use case an in the interface (the name of the keywords) is a more fruitful approach. The edge case is the need to have nullable ints and I think is easier to test, think and document that behavior. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
=======================================
Coverage 90.14% 90.14%
=======================================
Files 126 126
Lines 7211 7216 +5
=======================================
+ Hits 6500 6505 +5
Misses 711 711
Flags with carried forward coverage won't be shown. Click here to find out more.
|
fix #960