[RFC] changing python type by a term definition in docstring

[massich]

In the following docstring instead of using the python type as description, could we use a glossary term instead?

https://github.com/mne-tools/mne-python/blob/adb70865accfbf2e473d0f78e5be69c090581c0d/mne/io/meas_info.py#L205-L207

Or maybe we should keep the python type and link the term in the description.

    dig : list of dict | None
        The Polhemus :term:`digitization` data in head coordinates.
        See Notes for more information.

WDYT?

cc: @drammock, @larsoner, @agramfort

Maybe something like this could work:

numpydoc_xref_aliases = {
    'Popen': 'python:subprocess.Popen',
    'file-like': ':term:`file-like <python:file object>`',
    ...
    # MNE
    'Label': 'mne.Label', 'Forward': 'mne.Forward', 'Evoked': 'mne.Evoked',
    ...
    'Transform': 'mne.transforms.Transform',
    # MNE-terms
    'digitization': ':term:`digitization`',
}

Suggested term

digitization (abbr. dig) digpoint

Suggested definition

digitization: list of digpoints digpoint: Dictionary contaning ~Polhemus~ digitization data (kind, identifier, position, and coord_frame)

@drammock feel free to rephrase the whole thing ;) (or push me to do it)

[agramfort]

sounds good !

+1 to add a "digitization" entry in glossary

in the place you point to we should not mention Polhemus. it's not the only system to get a valid digitization.

[larsoner]

Xref only looks in the param list so no value in adding it if we're going to use it in the description

[massich]

@larsoner it was 2 different solutions to the same problem. Either one or the other but not both.

    dig : digitization | None
        The digitization object.

    dig : list of dict | None
        The ~Polhemus~ :term:`digitization` data in head coordinates.
        See Notes for more information.

I like the first one better.

[massich]

if its a digitization I've the right to just error say wrong type whatever.

[larsoner]

I would leave it as instance of Digitization in the type, what's wrong with doing it that way? Changing it to list of dict also has disadvantages, we need to explain what the dicts are. And we don't really want people thinking they can pass a list of dict and have it work. So I'm -1 on this, rather keep it as instance of Digitization.

You can keep the :term: in the description, though, if it helps

[agramfort]

I suggested to kill the Digitization class from the public API. I don't think we need to make it public as it will not replace the dig montage as originally thought when it was written

[larsoner]

The advantage of having it in the public API is that people don't think "I can just pass a list of dict" and I see little disadvantage to keeping it. What's the advantage to removing it?

[agramfort]

avoid to have yet another public class for little gain. It makes one more thing to explain to users. We can do the check that the dig is correct in the DigMontage constructor and the set_montage method. I am just trying to fight complexity here.

[drammock]

Regarding the new glossary entry: don't include the (abbr. dig) part in the head word line, it makes the crossrefs annoying to type. put it in the definition.

[larsoner]

Okay I can live with removing the class. It will hopefully also fix the spurious failures in Travis that occur due to our changing how __eq__ works. We'll probably want a little assert_dig_equal in tests instead

[massich]

On the assert_dig_equal I would do it the pytest way which would allow assert digA == digB

On Thu, Aug 22, 2019, 16:26 Eric Larson notifications@github.com wrote:

Okay I can live with removing the class. It will hopefully also fix the spurious failures in Travis that occur due to our changing how eq works. We'll probably want a little assert_dig_equal in tests instead

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/6698?email_source=notifications&email_token=ABVX5Y3RXLNKD2CFPSLZUQLQF2OZPA5CNFSM4IOUD6I2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45IRGY#issuecomment-523929755, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVX5Y32YEHDYH54N7STXFTQF2OZPANCNFSM4IOUD6IQ .

[massich]

We can use it as a test case, and if we like we would already have an example in the code base.

On Thu, Aug 22, 2019, 20:56 Sik mailsik@gmail.com wrote:

On the assert_dig_equal I would do it the pytest way which would allow assert digA == digB

On Thu, Aug 22, 2019, 16:26 Eric Larson notifications@github.com wrote:

Okay I can live with removing the class. It will hopefully also fix the spurious failures in Travis that occur due to our changing how eq works. We'll probably want a little assert_dig_equal in tests instead

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/6698?email_source=notifications&email_token=ABVX5Y3RXLNKD2CFPSLZUQLQF2OZPA5CNFSM4IOUD6I2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45IRGY#issuecomment-523929755, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVX5Y32YEHDYH54N7STXFTQF2OZPANCNFSM4IOUD6IQ .

[larsoner]

That is the way we do it now and it seems to cause spurious failures where Pytest tries to do some introspection and fails. Hence why I'm saying we should switch to something else.

Maybe just object_diff since it will give us the diff on failure of assert object_diff(x, y) == '' too.

[massich]

Damn! Now I remembered that it was not straight forward because it's a list.

Actually that would be the same root problem of the eq issue you mentioned. Maybe this would be a good excuse to fix it altogether. Inherit not from list and add the proper pytest == overload.

If I'm not mistaken I even try it. And decided that I didn't wanted to fight the inherit from list/dict battle.

On Thu, Aug 22, 2019, 20:57 Sik mailsik@gmail.com wrote:

We can use it as a test case, and if we like we would already have an example in the code base.

On Thu, Aug 22, 2019, 20:56 Sik mailsik@gmail.com wrote:

On the assert_dig_equal I would do it the pytest way which would allow assert digA == digB

On Thu, Aug 22, 2019, 16:26 Eric Larson notifications@github.com wrote:

Okay I can live with removing the class. It will hopefully also fix the spurious failures in Travis that occur due to our changing how eq works. We'll probably want a little assert_dig_equal in tests instead

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/6698?email_source=notifications&email_token=ABVX5Y3RXLNKD2CFPSLZUQLQF2OZPA5CNFSM4IOUD6I2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45IRGY#issuecomment-523929755, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVX5Y32YEHDYH54N7STXFTQF2OZPANCNFSM4IOUD6IQ .

[larsoner]

Now that we are getting rid of / making private Digitization it makes sense to keep it as close to list of dict as possible, rather than going in the opposite direction by making it a non-list class

[larsoner]

Closing for #6434

[larsoner]

Whoops I mean #6461