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

Ford documentation: improvements #784

Merged
merged 10 commits into from
Apr 8, 2024
Merged

Ford documentation: improvements #784

merged 10 commits into from
Apr 8, 2024

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Apr 2, 2024

Related to #783 by @ZedThree

Proposition:

  • Ford: upgrade from 6.1.10 to 7.0.5
  • exclude all BLAS/LAPACK source files from the ford processing (2 reasons: 1) too many procedures; 2) no support of both fypp and cpp by ford)
  • stdlib_str2num.fypp: small fixes for ford processing

@jvdp1 jvdp1 requested review from perazz and a team April 2, 2024 19:10
@jvdp1
Copy link
Member Author

jvdp1 commented Apr 2, 2024

@ZedThree it seems that this latest version of ford (7.0.5) does not generate the links between the specs to the code, e.g., for call [[stdlib_sorting(module):ord_sort(interface)]]( array[, work, reverse ] ). Has the notation been changed between 6.1.10 and 7.0.5?

Also, it seems that it is not possible to exclude files in 6.1.10. Hence, the need to upgrade ford to 7.0.5.

@gareth-nx
Copy link
Contributor

Thanks for this.

IMO it would be good to include documentation for the BLAS/LAPACK interfaces, if not the individual routines for each precision. Otherwise I think it will be very easy for users to become confused about what is available.

I'm not sure whether your pull request keeps the interfaces or not?

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 3, 2024

IMO it would be good to include documentation for the BLAS/LAPACK interfaces, if not the individual routines for each precision. Otherwise I think it will be very easy for users to become confused about what is available.

I'm not sure whether your pull request keeps the interfaces or not?

A description of BLAS/LAPACK is available in the specs. It was decided that only a link would be provided to the individual LAPACK interfaces.

However, I will try to find a way to process only one of the precisions. It could be indeed useful.

@gareth-nx
Copy link
Contributor

A description of BLAS/LAPACK is available in the specs. It was decided that only a link would be provided to the individual LAPACK interfaces.

Yes, to me it would be good to also have the available procedures in BLAS/LAPACK listed somewhere

However, I will try to find a way to process only one of the precisions. It could be indeed useful.

Totally agree, thanks.

@perazz
Copy link
Contributor

perazz commented Apr 3, 2024

These changes look great to me.
@jvdp1 @gareth-nx @ZedThree: In PR #786 I've addressed the FORD syntax issues, so, FORD can now process all BLAS and LAPACK sources just fine. However, I do confirm that it is a lengthy process (at least on my Mac).

I think that the best thing to do would be to exclude the sources, and have FORD only parse the interface blocks. However, FORD will fail in this case (because it cannot find the module procedures in the interface), so, this selective exclusion does not look viable.

@ZedThree
Copy link

ZedThree commented Apr 3, 2024

@ZedThree it seems that this latest version of ford (7.0.5) does not generate the links between the specs to the code, e.g., for call [[stdlib_sorting(module):ord_sort(interface)]]( array[, work, reverse ] ). Has the notation been changed between 6.1.10 and 7.0.5?

This should still work, I'll investigate

@ZedThree
Copy link

ZedThree commented Apr 3, 2024

@jvdp1 The issue with that particular example in stdlib_sorting.md is that it is within backticks:

##### Syntax

`call [[stdlib_sorting(module):ord_sort(interface)]]( array[, work, reverse ] )`

and in Ford v7 the priority has changed such that backticks now (correctly IMO) are always formatted verbatim, so this needs to be something like:

##### Syntax

`call` [[stdlib_sorting(module):ord_sort(interface)]] `( array[, work, reverse ] )`

A quick search shows about 270 examples of links within code blocks like this. A minimal change to ford to restore the old behaviour is:

modified   ford/_markdown.py
@@ -271,7 +271,7 @@ class FordLinkExtension(Extension):
     def extendMarkdown(self, md: MetaMarkdown):  # type: ignore[override]
         project = self.getConfig("project")
         md.inlinePatterns.register(
-            FordLinkProcessor(md, project=project), "ford_links", 174
+            FordLinkProcessor(md, project=project), "ford_links", 194
         )
 

but this would really need to be user-configurable, as otherwise it's not possible to write links verbatim (in general, nesting of inline markup is a bit of a nightmare of conflicting syntax, so this will always be a compromise).

If there's a strong desire for this, I can add this option and get it out in 7.0.6

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 3, 2024

Thank you @ZedThree for your explanations.

If the following syntax works with Ford v7, I can modify the specs within this PR to support it

##### Syntax

`call` [[stdlib_sorting(module):ord_sort(interface)]] `( array[, work, reverse ] )`

If there's a strong desire for this, I can add this option and get it out in 7.0.6

No need to revert it IMO. We can adapt these notations.

doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stats.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stats.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stats.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stats.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stats.md Outdated Show resolved Hide resolved
@jvdp1
Copy link
Member Author

jvdp1 commented Apr 5, 2024

@perazz @ZedThree @gareth-nx :

Currently, this PR does:

  • Ford: upgrade from 6.1.10 to 7.0.5
  • Fix all the specs to comply new Ford rules
  • Exclude all LAPACK source files from the Ford processing (reason: 1) too many procedures => too long)

Processing the BLAS files does not significantly influence the time of the CI.

I propose that we merge this PR as is, even if the LAPACK files are excluded for ford. The main reason is to reduce the time of the CI of other PR.

@perazz
Copy link
Contributor

perazz commented Apr 5, 2024

LGTM @jvdp1

@perazz
Copy link
Contributor

perazz commented Apr 5, 2024

If there was a way to include the LAPACK interfaces (stdlib_linalg_lapack.f90), without FORD looking for all module procedure implementations (stdlib_linalg_lapack_*.f90), it would be great, but to the best of my knowledge, it is not possible.

Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

Thanks @jvdp1, this looks good. I agree it makes sense to exclude Lapack until we can address the doc build times.

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 6, 2024

Thank you for your reviews.

If there was a way to include the LAPACK interfaces (stdlib_linalg_lapack.f90), without FORD looking for all module procedure implementations (stdlib_linalg_lapack_*.f90), it would be great, but to the best of my knowledge, it is not possible.

I tried several things similar to your proposition, but all failed! @ZedThree do you have maybe an idea? If not, I will wait a few more days and then merge it.

@ZedThree
Copy link

ZedThree commented Apr 8, 2024

There is the (slightly unintuitive) display: none option that stops the rendering of any children of an entity -- except that it seems we still actually process the docstring, we just don't display it, which doesn't help.

It should be possible to fix this, unfortunately I'm at a conference basically all this week, so it might take me awhile to look into it. A quick hack does show some improvement:

modified   ford/sourceform.py
@@ -612,6 +612,10 @@ class FortranBase:
     def filter_display(self, collection: Sequence) -> List:
         """Remove items from collection if they shouldn't be displayed"""
 
+        for obj in collection:
+            if not self._should_display(obj):
+                self.source_file._to_be_markdowned.remove(obj)
+
         return [obj for obj in collection if self._should_display(obj)]

but there still seems to be a pretty big slowdown.

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 8, 2024

Thank you @ZedThree for your comment. There is no rush for this issue IMO. I will merge this PR as is.
You can look at this issue later if you want, and when you have time.

@jvdp1 jvdp1 merged commit 4ed4c52 into fortran-lang:master Apr 8, 2024
17 checks passed
@jvdp1 jvdp1 deleted the fix_ford branch April 8, 2024 10:34
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.

4 participants