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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify Target of reference_url #947

Closed
ExtremOPS opened this issue Apr 20, 2022 · 20 comments
Closed

Specify Target of reference_url #947

ExtremOPS opened this issue Apr 20, 2022 · 20 comments

Comments

@ExtremOPS
Copy link
Contributor

Hi there,

we are using sphinx-gallery for our OSAFT project and are very happy 馃樃

I wanted now to add the option of reference_url and did so by adding

# sphinx_gallery
sphinx_gallery_conf = {
    ...
    # Modules for which function/class level galleries are created.
    'doc_module': ('osaft'),
    'reference_url': {
        'osaft': None,
    },
}

For some reasons the inheritance diagram for the classes of interest look like this:

image

The common feature is that all ARF classes inherit from BaseARF. The issue that I have is visible here. In the code block where it reads

 yosioka = osaft.yosioka1955.ARF(
     frequency=f,
     radius=R_0,
     ...
)

Does a "click" on osaft.yosioka1955.ARF, osaft.gorkov1962.ARF, or osaft.settnes2012.ARF transfer you to the documentation page of osaft.solutions.BaseARF instead of the anticipated osaft.solutions.yosioka1955.ARF.

I hope that I expressed the issue well enough! Is there a possibility to set the depth of the reference or similar.

Otherwise, I am also happy to add a patch enabling it if you could point me to the lines of code where this would need to be implemented.

Than you very much in advance,
Christoph

@larsoner
Copy link
Contributor

I think the resolution happens somewhere in this class:

class SphinxDocLinkResolver(object):

Debugging is not easy, and the code is not so easy to follow, but please do give it a shot!

@ExtremOPS
Copy link
Contributor Author

Thank you @larsoner. After playing around a little I figured that somehow this line causes the problem:

    first, second = cobj['module_short'], cobj['name']

If I change it to

    first, second = cobj['module'], cobj['name']

it solves my problem and does not lead to an error. Do you happen to know why it is 'module_short' instead of 'module'?

@larsoner
Copy link
Contributor

Not offhand, I have to work to re-understand what that code does every time I need to change it :(

@ExtremOPS
Copy link
Contributor Author

I thought so. 馃槃 That is how I usually feel if I have to re-examine my code

image

The question is if multiple user have the same problem or if it is just me... I could think of an option which you can set in conf.py where you specify which ('module' or 'module_short') you want to use. But then, if it is just us with the issue then it probably does not make sense to put it into the production code.

@larsoner
Copy link
Contributor

It's likely that other users have this problem, too. But it's tough to know if changing it will break things for them...

What are module and module_short in your case?

@ExtremOPS
Copy link
Contributor Author

E.g. it is:

{..., 'module': 'osaft.solutions.yosioka1955.arf', 'module_short': 'osaft.solutions.yosioka1955',...}

instead of yosioka1955 it can also be another name. It seems like that for some reason it strips of the last module path .arf

@larsoner
Copy link
Contributor

I think module_short is preferred because it's more common that a class is defined in a deeper module, but documented in a shallower one, e.g. a class mne.evoked.Evoked gets an API page mne.Evoked because it's imported in mne.__init__ (and that's the namespace where users expect it). I think you're in a less common situation where your base class osaft.solutions.BaseARF is at a shallower level than osaft.solutions.yosioka1955.ARF.

Does the problem go away if you move the BaseARF in your API page at least to be documented in osaft.solutions.base.BaseARF so it's in a different, non-parent namespace of ARF? I assume this isn't a good solution at your end since it will require actually changing your namespaces, but it will tell us if this idea is likely to be on the right track.

We can start thinking about workarounds, though. One option would be to have a prefer_module=set() that you can change to prefer_full_module={'osaft.solutions.yosioka1955.ARF'}, and in SG we look to see if the current name (or whatever the full key would be) is in prefer_full_module and if so, look at module first rather than module_short. Or something like this...

@ExtremOPS
Copy link
Contributor Author

I put all our base classes one folder deeper as you suggested to check if that would solve the problem. However it does not. 馃槥

I changed the links in all python as well as all rst files and checked with our unittests that the change of namespace was done properly. I built then the documentation by running make clean html in order to delete all old files. I also made sure that I reloaded the webpage and cleared the cache.

Regarding you workarounds, I will need some more time to understand what you mean 馃槃 I happy to help then with possible implementations.

@ExtremOPS
Copy link
Contributor Author

It feels like that somehow SG is looking for the superclasses instead of the current one. I tried the following: I removed all Base* classes under osaft.solutions from being documented with the hope that now the link should be right. But instead of linking to, e.g., osaft.solutions.yosioka1955.arf.ARF it links now to BaseFrequencyComposite which is another class where it inherits from...

Regarding your suggestions for a workaround. I think it should then also support wildecards such that you can do prefer_full_module={'osaft.solutions.*.ARF'} otherwise it might be quite cumbersome to write them all by hand.

Would it not just work to have prefer_full_module without having prefere_module and whatever name is in the prefer_full_module will use module instead of module_short?

@larsoner
Copy link
Contributor

instead of linking to, e.g., osaft.solutions.yosioka1955.arf.ARF it links now to BaseFrequencyComposite which is another class where it inherits from...

This is very strange. Before we continue, does the page of the super class in the API docs actually have the method? If not, you might just need autodoc_default_options = {'inherited-members': None} in your conf.py like:

https://github.com/mne-tools/mne-python/blob/0a07c48fe3f21ee2e9419414d69db95ac881a160/doc/conf.py#L522

Regarding your suggestions for a workaround. I think it should then also support wildecards such that you can do prefer_full_module={'osaft.solutions.*.ARF'} otherwise it might be quite cumbersome to write them all by hand.

We usually support regex (or list of regex that we internally concatenate to a single regex)

Would it not just work to have prefer_full_module without having prefere_module and whatever name is in the prefer_full_module will use module instead of module_short?

It might. But I suspect this will lead to problems with other classes then not getting their methods documented properly.

@ExtremOPS
Copy link
Contributor Author

ExtremOPS commented Apr 21, 2022

This is very strange. Before we continue, does the page of the super class in the API docs actually have the method? If not, you might just need autodoc_default_options = {'inherited-members': None} in your conf.py like:

That was a bad explanation from my side. I tested it again it is like that:

In this example I create a class in line 71 with

yosioka = osaft.yosioka1955.ARF(
     frequency=f,
     radius=R_0,
     rho_s=rho_s, c_s=c_s,
     rho_f=rho_f, c_f=c_f,
     p_0=p_0,
     wave_type=wave_type,
     position=position,
 )

If I click on osaft.yosioka1955.ARF it directs me to BaseARF. If I delete all base classes from being documented then it directs me to BaseFrequencyComposite , yet another parent of BaseFrequencyComposite.

But, clicking on print(f'{yosioka.compute_arf() = :.3e}') in line 118 does not work anymore after deleting all Base... documentation.

Sorry the incomplete explanation!

I created a pullrequest where I implemented it with this in mind

Would it not just work to have prefer_full_module without having prefere_module and whatever name is in the prefer_full_module will use module instead of module_short?

and it works for our specific example. At least for our documentation I have not found a place yet where the documentation leads to wrong pages.

@larsoner
Copy link
Contributor

I have not found a place yet where the documentation leads to wrong pages.

If it simplifies things a lot then we can just stick with a bool. But generally it's only an extra line or two to add list-of-regex support, and easy enough for people to do '.*' if they want "all".

@ExtremOPS
Copy link
Contributor Author

Mhm. I am happy to adapt to your changes. In the end, you have more experience what is the style of SG.

As said, I started an implementation in #950 and if could point me please to the portion I would need to change/add, I am happy to do so.

@ExtremOPS
Copy link
Contributor Author

@larsoner I wanted to ask if and how we could progress with this issue here.

@larsoner
Copy link
Contributor

To make it easy I would make it just be a single regex to use like '.*' to mean "all" and the default '' to mean "none". Then you just re.match with the currently checked class name and prefer module over module_short if it matches

@ExtremOPS
Copy link
Contributor Author

Thanks for your fast reply. If we do a binary options then I could just do

...
'prefer_full_module': False,
...

and the user can set it to True if wanted.

At the moment I implemented it in #950 as for loop over all items within prefer_full_module.

for pattern in self.config['prefer_full_module']:
     if re.search(pattern, full_name):
         use_full_module = True
         break

If we the binary option then I would change it to

...
use_full_module = self.config['prefer_full_module']
...

Did I understand your suggestions in the right way?

@larsoner
Copy link
Contributor

At the moment I implemented it in #950 as for loop over all items within prefer_full_module.

This seems fine to me!

@ExtremOPS
Copy link
Contributor Author

All right then. I'll add some documentation about it and inform you when all is ready. I appreciate all your time and the suggestions 馃憤馃徑

@ExtremOPS
Copy link
Contributor Author

I added now some documentation text. However, it is not easy to explain what it actually does, so I added a link to this issue giving (hopefully) more context.

@ExtremOPS
Copy link
Contributor Author

Hi @larsoner, I just wanted to check in if my edits are fine, If there are any suggestions you have for me I happy to work in it. Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants