[MRG] ENH: Added instance type check for plot_dipole function and also a test for it #606

raj1701 · 2023-03-06T14:56:31Z

Closes #511
I made changes in plot_dipole function in viz.py to type of argument 1. Iteration is used when a list of dipoles is passed. I have included a test in test_dipole.py where [dipole, integer] is passed as an argument to plot_dipole() and the appropriate error message is checked using pytest.

raj1701 · 2023-03-07T10:06:14Z

I have removed the flake8 errors

jasmainak · 2023-03-07T15:52:56Z

hnn_core/viz.py

+    else:
+        if not isinstance(dpl, Dipole):


do you need to nest here? Zen of Python:

Flat is better than nested

do you ever enter this condition? See above in L271 ... it is turned into a list

jasmainak · 2023-03-07T15:55:16Z

hnn_core/viz.py

@@ -273,6 +273,18 @@ def plot_dipole(dpl, tmin=None, tmax=None, ax=None, layer='agg', decim=None,
    elif average:
        dpl = dpl + [average_dipoles(dpl)]

+    if type(dpl) == list:


use isinstance for type checks ... and in any case object equality is not done using ==. You should use is.

Sorry @jasmainak , I missed the L271. This condition is redundant and also the else block. I have updated the changes. Please check them

jasmainak · 2023-03-07T15:58:56Z

You should use the phrase "closes #511", otherwise the linked PR will not get closed when this PR is merged. See:

https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

codecov-commenter · 2023-03-07T18:38:49Z

Codecov Report

Merging #606 (f62a159) into master (d5d1a03) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head f62a159 differs from pull request most recent head fc1e84c. Consider uploading reports for the commit fc1e84c to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
- Coverage   92.19%   92.12%   -0.07%     
==========================================
  Files          22       22              
  Lines        4229     4231       +2     
==========================================
- Hits         3899     3898       -1     
- Misses        330      333       +3

Impacted Files	Coverage Δ
hnn_core/viz.py	`89.23% <100.00%> (+0.04%)`	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jasmainak · 2023-03-07T19:42:24Z

hnn_core/viz.py

@@ -273,6 +273,11 @@ def plot_dipole(dpl, tmin=None, tmax=None, ax=None, layer='agg', decim=None,
    elif average:
        dpl = dpl + [average_dipoles(dpl)]

+    for single_dpl in dpl:
+        if not isinstance(single_dpl, Dipole):
+            raise ValueError('Arg 1 should be of type dipole or list of dipol'


say dpl instead of Arg 1 since that's what we call it in the function signature. And nitpick but dipole -> Dipole since class names are always capitalized in PEP convention

Sure will make these changes

jasmainak · 2023-03-07T19:42:45Z

Can you update whats_new.rst ?

raj1701 · 2023-03-08T04:42:02Z

Should I update in Current Changelog in whats_new.rst?

jasmainak · 2023-03-08T14:54:00Z

doc/whats_new.rst

@@ -32,6 +32,9 @@ Changelog
 - Add ability to record voltages and synaptic currents from all sections in :class:`~hnn_core.CellResponse`,
  by `Nick Tolley`_ in :gh:`502`.

+- Add ability to check instance type of Dipole in :func:`~hnn_core.viz.plot_dipole`,


this is more of a bugfix I would say ... but this will not get rendered properly. Did you check how to build the documentation in the contributing guide:

https://jonescompneurolab.github.io/hnn-core/dev/contributing.html

Sure I will put it in bugfix and check the rendering properly

Yes, look at the bottom of the page. See also: https://sublime-and-sphinx-guide.readthedocs.io/en/latest/references.html#links-to-external-web-pages

Hey @jasmainak I made all changes. Please check them once

doc/whats_new.rst

jasmainak

good by me. @rythorpe @ntolley merge button is yours. Please "squash and merge" since there are merge commits in the history.

rythorpe · 2023-03-09T19:56:15Z

hnn_core/viz.py

+            raise ValueError('dpl should be of type Dipole or list of Dipole, '
+                             f'but {single_dpl} is a {type(single_dpl)}')


If single_dpl happens to be a list or ndarray, this error message will be long and potentially hard to read.

What alternative do you suggest moving forward here?

Suggested change

raise ValueError('dpl should be of type Dipole or list of Dipole, '

f'but {single_dpl} is a {type(single_dpl)}')

raise ValueError('dpl should be of type Dipole or list of Dipole '

f'but is of type {type(single_dpl)}')

We just don't want the traceback to be huge if the user accidentally enters dpl as a list of list or ndarray.

hnn_core/viz.py

doc/whats_new.rst

rythorpe · 2023-03-09T20:03:54Z

Also, you'll need to rebase @raj1701. Let me know if you need help with this.

raj1701 · 2023-03-09T20:09:43Z

I can give it a shot. I'll contact you if I get stuck.

raj1701 · 2023-03-09T21:07:45Z

@rythorpe I thinks I was successful with the rebase. Please check once. Thanks

rythorpe · 2023-03-09T23:03:00Z

hnn_core/viz.py

+                raise ValueError('dpl should be of type Dipole, '
+                                 f'but {single_dpl } is a {type(single_dpl )}')


Suggested change

raise ValueError('dpl should be of type Dipole, '

f'but {single_dpl } is a {type(single_dpl )}')

raise ValueError('dpl should be of type Dipole or list of Dipole, '

f'but is a list containing type {type(single_dpl)}')

hnn_core/viz.py

rythorpe · 2023-03-09T23:06:23Z

You're almost done @raj1701, just a minor adjustment to the warning message!

rythorpe · 2023-03-10T14:37:10Z

hnn_core/viz.py

+            if not isinstance(single_dpl, Dipole):
+                raise ValueError('dpl should be of type Dipole or list of '
+                                 'Dipole, but is a list containing '
+                                 f'type {type(single_dpl )}')


Suggested change

f'type {type(single_dpl )}')

f'type {type(single_dpl)}')

This is nitpicky, but we really want to keep the code as clean and readable as possible.

Removed extra spaces

rythorpe · 2023-03-10T14:37:24Z

hnn_core/viz.py

+            dpl = dpl + [average_dipoles(dpl)]
+    else:
+        raise ValueError('dpl should be of type Dipole, '
+                         f'but is a type {type(dpl )}')


Suggested change

f'but is a type {type(dpl )}')

f'but is a type {type(dpl)}')

Also, I guess we should update this one to say "dpl should be of type Dipole or list of Dipole[...]" as we do above so that users don't get conflicting messages when errors pop up.

Actually, I just realized why don't we use _validate_type here like in the rest of the codebase? So we're not reinventing the wheel

So instead of using isinstance(), _validate_type should be used ?

those are mostly doing other operations than type-checks after the isinstance ... in general, the smaller your diff is, the better. Larger diff and logic means we have to check carefully to make sure there are no corner cases. In this case, if _validate_type works, you should use it. It is also more DRY

+1 for _validate_type - I totally forgot about that function.

Ok will add _validate_type in the code

jasmainak · 2023-03-10T20:20:38Z

hnn_core/viz.py

+    elif isinstance(dpl, list):
+        for single_dpl in dpl:
+            _validate_type(single_dpl, Dipole, 'dpl', 'Dipole, list of Dipole')
+        if average:
+            dpl = dpl + [average_dipoles(dpl)]
+    else:
+        _validate_type(dpl, Dipole, 'dpl', 'Dipole, list of Dipole')


do you even need to do this? Can't you just do:

_validate_type(dpl, Dipole, 'Dipole, list of Dipole')

?

In the earlier code, there already was a else if ladder :
if isinstance(dpl, Dipole):
dpl = [dpl]
elif average:
dpl = dpl + [average_dipoles(dpl)]

-Now the 'dpl' can be of type Dipole or list of Dipole.

I think if we can pass multiple types to _validate_type() then we can do type checking in one go else we will be needing a similar else-if code.

Therefore I merged the two together.

The line 279 code is simply to point error. If the control reaches line 279, the type passed is always invalid. It is just a corner case where type error needs to be raised. eg- dpl is an integer 10 then control will reach line 279

I can try passing dipole and list of Dipole as types to _validate_type() like this
_validate_type(dpl,[Dipole, [Dipole]],'Dipole, list of Dipole')).
I will try this and let you know the results.
If there is some other way then please let me know

I can pass types = [Dipole,list] only. Is there a way to pass list of Dipoles as an argument? I tried with [Dipole] and list(Dipole) but both didn't work

jasmainak · 2023-03-14T20:18:44Z

Okay I simplified the logic a bit @raj1701 @rythorpe ... please take a look. It is slightly different from the original logic but I think it's not less correct. Wdyt?

raj1701 · 2023-03-14T20:32:53Z

I think its correct and much more simplified. Now I get what you were trying to convey @jasmainak.

rythorpe · 2023-03-14T22:02:26Z

hnn_core/viz.py

+        _validate_type(this_dpl, Dipole, 'dpl', 'Dipole, list of Dipole')
+
+    if average:
        dpl = dpl + [average_dipoles(dpl)]


@jasmainak what if someone happens to pass in a numpy array of Dipole objects? This would be an easy mistake to make and would pass all of our checks, but would get weird on L277 when the averaging step assumes dpl + [average_dioles(dpl)] is an appending operation.

@rythorpe good catch! How about making use of duck typing? Unfortunately the numpy append does not work the same way as a list ... it does not work in-place. In MNE, we start most functions by saying arr = np.array(arr) to normalize lists and arrays, and then work pretending it's an array. That ship seems to have sailed here though. I could also place an explicit error if you prefer.

…ing test case

Fixed errors Fixed rebase errors

jasmainak · 2023-03-16T15:13:00Z

@raj1701 try not to make too many commits for a simple change. It will make your life difficult with respect to rebasing. Make use of git commit --amend or squash the commits so each commit represents a logical unit of code.

rythorpe · 2023-03-16T20:06:52Z

Are we ready to rebase and merge @raj1701 @jasmainak?

raj1701 · 2023-03-16T20:41:04Z

@raj1701 try not to make too many commits for a simple change. It will make your life difficult with respect to rebasing. Make use of git commit --amend or squash the commits so each commit represents a logical unit of code.

Yes will remember this. I faced many rebase errors here also.

jasmainak · 2023-03-16T20:49:29Z

Yep, I changed the PR to MRG. Please squash and merge @rythorpe !

rythorpe · 2023-03-16T22:16:30Z

Thanks @raj1701 and @jasmainak!

raj1701 changed the title ~~Added instance type check for plot_dipole function and also a test for it~~ ENH: Added instance type check for plot_dipole function and also a test for it Mar 7, 2023

jasmainak reviewed Mar 7, 2023

View reviewed changes

jasmainak reviewed Mar 8, 2023

View reviewed changes

doc/whats_new.rst Outdated Show resolved Hide resolved

jasmainak approved these changes Mar 8, 2023

View reviewed changes

rythorpe reviewed Mar 9, 2023

View reviewed changes

hnn_core/viz.py Outdated Show resolved Hide resolved

rythorpe reviewed Mar 9, 2023

View reviewed changes

doc/whats_new.rst Outdated Show resolved Hide resolved

rythorpe reviewed Mar 9, 2023

View reviewed changes

doc/whats_new.rst Show resolved Hide resolved

rythorpe reviewed Mar 9, 2023

View reviewed changes

hnn_core/viz.py Outdated Show resolved Hide resolved

rythorpe reviewed Mar 10, 2023

View reviewed changes

jasmainak reviewed Mar 10, 2023

View reviewed changes

rythorpe reviewed Mar 14, 2023

View reviewed changes

raj1701 added 2 commits March 16, 2023 11:01

Added instance type check for plot_dipole function and also a support…

bc23ec5

…ing test case

Refactored code to pass tests

3087d92

raj1701 and others added 11 commits March 16, 2023 11:01

Removed redundant condition

49090c6

Fixed error display message and added to whats_new.rst

e4d53ba

Updated whats_new.rst

c3d7132

Updated whats_new.rst

d3fc290

Removed rebase errors

83a1573

Updated whats_new.rst

ee7891a

Fixed errors Fixed rebase errors

Updated functionality

ebc380d

Updated error message

f54312b

Removed additional spaces

0b4364b

Switched to _validate_type

6a91a80

MAINT: simplify

7ade37a

jasmainak force-pushed the plot_dipole_instance_check branch from 194ca10 to efd4d04 Compare March 16, 2023 15:06

MAINT: duck typing?

fc1e84c

jasmainak force-pushed the plot_dipole_instance_check branch from efd4d04 to fc1e84c Compare March 16, 2023 15:14

rythorpe approved these changes Mar 16, 2023

View reviewed changes

jasmainak changed the title ~~ENH: Added instance type check for plot_dipole function and also a test for it~~ [MRG] ENH: Added instance type check for plot_dipole function and also a test for it Mar 16, 2023

rythorpe merged commit 664a5ad into jonescompneurolab:master Mar 16, 2023
8 checks passed

		raise ValueError('dpl should be of type Dipole or list of Dipole, '
		f'but {single_dpl} is a {type(single_dpl)}')

		raise ValueError('dpl should be of type Dipole, '
		f'but {single_dpl } is a {type(single_dpl )}')

[MRG] ENH: Added instance type check for plot_dipole function and also a test for it #606

[MRG] ENH: Added instance type check for plot_dipole function and also a test for it #606

Conversation

raj1701 commented Mar 6, 2023 • edited

raj1701 commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raj1701 Mar 7, 2023 • edited

Choose a reason for hiding this comment

jasmainak commented Mar 7, 2023

codecov-commenter commented Mar 7, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Mar 7, 2023

raj1701 commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Mar 9, 2023

raj1701 commented Mar 9, 2023

raj1701 commented Mar 9, 2023

Choose a reason for hiding this comment

rythorpe commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raj1701 Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Mar 14, 2023

raj1701 commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Mar 16, 2023

rythorpe commented Mar 16, 2023

raj1701 commented Mar 16, 2023

jasmainak commented Mar 16, 2023

rythorpe commented Mar 16, 2023

raj1701 commented Mar 6, 2023 •

edited

raj1701 Mar 7, 2023 •

edited

codecov-commenter commented Mar 7, 2023 •

edited

jasmainak Mar 10, 2023 •

edited

raj1701 Mar 10, 2023 •

edited