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

[WIP] Added API link in examples #1013

Merged
merged 19 commits into from
Jan 26, 2020
Merged

Conversation

percygautam
Copy link
Contributor

@percygautam percygautam commented Jan 19, 2020

Fixes #1000 . Added API link in the examples.

@percygautam
Copy link
Contributor Author

@OriolAbril The link is showing in generated docs when I use make html. But I don't how to check whether link will work in github or not?

Comment on lines 242 to 245
def apiname(self):
name = self.modulename.split("_")
name = name[1::]
return "_".join(name)
Copy link
Member

Choose a reason for hiding this comment

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

I think this will fail in many cases such as {backend}_plot_ess_evolution (or quantile or local), {backend}_plot_loo_pit_ecdf, {backend}_plot_pair_hex (or kde) and so on. There are several plots with more than one image in the gallery were some extra description is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OriolAbril I'll try to come up with a solution.

@aloctavodia aloctavodia changed the title [WIP] Solve issue #1000 : Added API link in examples [WIP] Added API link in examples Jan 20, 2020
Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

Added some comments

doc/sphinxext/gallery_generator.py Show resolved Hide resolved
name = self.modulename.split("_")
name = name[1::]
return "_".join(name)
name=""
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just return None if nothing is found?

Copy link
Contributor Author

@percygautam percygautam Jan 21, 2020

Choose a reason for hiding this comment

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

Okay, make sense.

regex = r"az\.(plot\_[a-z_]+)\("
matches = re.finditer(regex, file.read(), re.MULTILINE)
for matchNum, match in enumerate(matches, start=1):
name = match.group(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this return the last found plot_xyz? Should it return all the matches?

Do you think re.findall would work here? No need for looping?

Copy link
Member

Choose a reason for hiding this comment

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

I think the first is enough, there is only a couple of cases where more than a function is called, and in these cases the function called is always the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahartikainen @OriolAbril I worked some examples and let's say we have a hypothetical code as:

az.plot_compare(model_compare, figsize=(12, 4))
az.plot_forest(model_compare, figsize=(12, 4))
az.plot_density(model_compare, figsize=(12, 4))

If we run our code, it will return the last found match as 'plot_density'. As in our example files, we are only calling the function one time, so I don't think it'll be a problem.
As a possible solution for, when (in future) we have multiple plots in a single example, we can do something like:

    for matchNum, match in enumerate(matches, start=1):
        api_name.append(match.group(1))
    return api_name #returns the list

And then maybe check the length when rendering it in the template.
As suggested by @ahartikainen , re.findall() will work best for such case. For example, above code can be simplified as

with open("mpl_plot_compare.py", "r") as f:
    regex = r"az\.(plot\_[a-z_]+)\("
    api_name = re.findall(regex, f.read())
return api_name  #api_name will contain list

How should we do it?

Copy link
Member

Choose a reason for hiding this comment

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

I think using only the first function is good enough for that. Returning the list with all occurrences seems like an overkill which complicates the code without much gain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll do it.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Looks good, I only have one question, what happens to the styles example?

@percygautam
Copy link
Contributor Author

percygautam commented Jan 21, 2020

@OriolAbril It would return API Documentation: None, with a link to az.None function which will give file not found error. I tried to give some condition to rst template so that it does not show the API documentation line, but supposedly rst doesn't support inline code snippets

@@ -40,6 +40,7 @@ def execfile(filename, globals=None, locals=None):
.. image:: {img_file}

**Python source code:** :download:`[download source: {fname}]<{fname}>`
**API documentation:** `{api_name} <../../generated/arviz.{api_name}>`_
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could be changed to {api_text} so that below, if api_name is None nothing is written?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OriolAbril , I have done the changes, now it'll show None <None> and not the directory link.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more clear replacing the whole line, so that when no match is fount (e.g. styles case) instead of **API doc... a white line is written. Or alternatively something like:

API Documentation: No API Documentation available for {api_name}

I see myself some months from now confusing the None <None> with a doc generation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OriolAbril I understand your point. I have done the changes. Now, it'll show something like:

API Documentation: No API Documentation available

We can't specify the name of documentation available because api_name would be an open string and other params like modulename will be in the form of backend_xyz. If we must do this then we have to use regex again or split the name and then pass to the new variable.

with open(op.join(self.target_dir, self.pyfilename), "r") as file:
regex = r"az\.(plot\_[a-z_]+)\("
name = re.findall(regex, file.read())
apitext = name[0] if len(name) > 0 else ""
Copy link
Member

Choose a reason for hiding this comment

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

I think if name acts the same as if len(name) > 0

name = re.findall(regex, file.read())
apitext = name[0] if len(name) > 0 else ""
return (
"`" + apitext + " <../../generated/arviz." + apitext + ">`_"
Copy link
Member

Choose a reason for hiding this comment

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

.format can be used here instead of adding strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll add it.

@OriolAbril
Copy link
Member

@percygautam #990 has already been merged, so rebasing/merging the latest changes in master should fix the failing tests. After this it can be merged

@percygautam
Copy link
Contributor Author

Okay, Thanks.

@OriolAbril OriolAbril merged commit 06f0153 into arviz-devs:master Jan 26, 2020
@percygautam percygautam deleted the API_link branch January 26, 2020 02:37
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

Successfully merging this pull request may close these issues.

Show API link en examples
7 participants