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

Changed __repr__ in BinnedSpikeTrain to support quantities<0.12.4 #418

Merged
merged 1 commit into from
May 21, 2021

Conversation

kohlerca
Copy link
Collaborator

The unit test for method __repr__ in conversion.BinnedSpikeTrain fails if quantities is 0.12.3 or below.

The old method was using Quantity scalars inside an f-string. This required the __format__ method implemented in quantities==0.12.4 to generate the correct string representation of BinnedSpikeTrain with magnitudes and units.

This PR changes the string generation by casting the Quantity scalars to string, which uses the __str__ method in Quantity and displays the magnitudes and units as expected. This does not rely on any formatting method and thereby it is compatible with older versions of quantities.

This addresses issue #409.

@coveralls
Copy link
Collaborator

Coverage Status

Coverage remained the same at 88.82% when pulling 3165aeb on INM-6:fix/binned_spike_train_repr into 165276f on NeuralEnsemble:master.

@mdenker mdenker self-assigned this May 17, 2021
@mdenker mdenker requested a review from ojoenlanuca May 17, 2021 10:22
@mdenker mdenker added the bugfix Fix for an indentified bug. label May 17, 2021
Copy link
Collaborator

@ojoenlanuca ojoenlanuca left a comment

Choose a reason for hiding this comment

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

I can approve these changes.
Little side-note: it is maybe IDE dependent, but when i run test_conversion.test_repr in Pycharm to recreate the bug #409, the self.assertEqual() function might expect as first parameter the desired value and secondly the actual value. Because in the respective debug-message, the order is switched to
Expected: string generated by repr()
Actual: hardcoded reference string

@kohlerca
Copy link
Collaborator Author

This is a PyCharm issue, as it is known that its diff tool provides these expected/actual reports in an order different than the one used in several of our tests (https://youtrack.jetbrains.com/issue/PY-26471), i.e., assuming that the expected value is the first. There is a recommendation by Guido van Rossum against forcing this, and instead, refer to the values in a generic form, such as first/second (https://mail.python.org/pipermail/python-dev/2010-December/106954.html). This is actually what the assert function documentation uses.

If running the tests using the command line, the standard != and the points with the differences are shown without any semantics attached:

E       AssertionError: 'Binn[19 chars]t=1.0, t_stop=1.01, bin_size=0.001; shape=(1, [19 chars]rix)' != 'Binn[19 chars]t=1.0 s, t_stop=1.01 s, bin_size=0.001 s; shap[25 chars]rix)'
E       - BinnedSpikeTrain(t_start=1.0, t_stop=1.01, bin_size=0.001; shape=(1, 10), format=csr_matrix)
E       + BinnedSpikeTrain(t_start=1.0 s, t_stop=1.01 s, bin_size=0.001 s; shape=(1, 10), format=csr_matrix)
E       ?                             ++             ++                ++

@ojoenlanuca
Copy link
Collaborator

Ok, so then i can approve the pull-request.

Copy link
Collaborator

@ojoenlanuca ojoenlanuca left a comment

Choose a reason for hiding this comment

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

These changes prevent test_converstion.test_repr()-failure for quantities version <= 0.12.3 and should be adopted.

@mdenker mdenker added this to the v0.11.0 milestone May 21, 2021
@mdenker mdenker merged commit d407863 into NeuralEnsemble:master May 21, 2021
@Moritz-Alexander-Kern Moritz-Alexander-Kern mentioned this pull request Mar 9, 2022
1 task
@Moritz-Alexander-Kern Moritz-Alexander-Kern deleted the fix/binned_spike_train_repr branch October 28, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix for an indentified bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants