search

lspestrip / striptease

Strip TEst Analysis for System Evaluation
MIT License
4 stars 3 forks source link

Fix conversion from unsigned to signed in load_sci #91

Closed teozec closed 1 year ago

teozec commented 1 year ago

Hi. This PR aims at fixing the problem in the DataFile.load_sci() function, which failed at converting unsigned data to signed. This seems to be due to the h5py library, therefore I moved the conversion after the column selection, which returns a numpy array. This allows us to use the .astype() method from numpy instead of the one from h5py. At the same time, I removed the now useless check for overflow (a quick search in the striptease library shows that it is not used anywhere in our programs).

Let me know if you prefer to leave a dummy check_overflow parameter to preserve compatibility with any possible program that is not in striptease and I am not aware of.

Thanks!

ziotom78 commented 1 year ago

Hi @teozec , oops! I removed check_overflow this morning because I missed this PR from yours… For me it's fine to get rid of check_overflow completely, keeping it active would just confuse users.

teozec commented 1 year ago

Ok perfect, thanks! Should I also, while we are touching this function, remove the

if not detector:
    detector = ["Q1", "Q2", "U1", "U2"]

check and simply use a default parameter detector = ["Q1", "Q2", "U1", "U2"]? I think it would break compatibility only if someone, instead of not providing the parameter, called the function with detector=[], None, False etc.

ziotom78 commented 1 year ago

Ok perfect, thanks! Should I also, while we are touching this function, remove the

if not detector:
    detector = ["Q1", "Q2", "U1", "U2"]

check and simply use a default parameter detector = ["Q1", "Q2", "U1", "U2"]? I think it would break compatibility only if someone, instead of not providing the parameter, called the function with detector=[], None, False etc.

I checked the code and it does not seem that detector=None is used anywhere, so ok!

teozec commented 1 year ago

Done! Let me know if I should change anything else. Thanks.