Update hyperlinks in documentation#2495
Conversation
|
@felixhekhorn I decided to also fix the equation references in https://docs.nnpdf.science/theory/PTevol.html so in the end it will still take some time |
|
wait a moment: I suggest to drop that page as is and instead point to https://eko.readthedocs.io, where all of this is already explained in great detail @scarlehoff do you agree? |
|
cc #2288 |
|
It's the information exactly the same? If yes, ok from me. |
|
@felixhekhorn Ah ok, I had not seen that in #2288 this stuff is already being done. Shall I check if the info is the same & drop it if yes, or leave it to #2288? |
all the evolution information is covered by EKO - and actually this page here is severely outdated, e.g. it does not discuss QED, that we use exact these days, or SV scheme B, so better we drop it. And yes, @evagroenendijk please drop it here and then tick off the relevant box over there Instead, TMC - which has nothing to do with evolution - should point to https://yadism.readthedocs.io/en/latest/implementation/TMC.html ... aaand then that page is empty, but then we should fix that there, where it belongs. The current content can not be simply transferred since it uses Mellin space (which yadism is not doing). Also the implementation in yadism is more complex as we allow, e.g., different levels (/orders) if TMC. Note that the code documentation does have some explanation ... Any way, don't forget the main purpose of this PR 🙃 |
felixhekhorn
left a comment
felixhekhorn
left a comment
There was a problem hiding this comment.
As I already said: updating the documentation is nice and good - but was not the intended task for this PR
Sure! That's why I did not look at the text, I just updated the links. Actually as far as the nnpdf documentation goes, I pushed all the changes I made already (I did not find any other deprecated links) |
ehm, actually, looking at 28ae19b let me ask to be sure: is it clear what #572 wants to do? Specifically, we are not talking about any explicitly written URL in our documentation (like the ones you change in that commit) - we are talking about the automatic link generated by sphinx
(maybe this indicates we are not talking about the same thing 🙈 ) |
Ah okay, then indeed I misunderstood the issue... |
|
@felixhekhorn I think I can use this https://www.sphinx-doc.org/en/master/usage/extensions/linkcode.html then to solve the issue, would you agree? It can then point to the n3fit and validphys directories in the repository |
not sure - this is the right direction, but then you would need to provide the mapping between name of "the thing" (function, class) and the URL yourself. I hope there are better solutions out there, which are already targeted at GitHub and do all the magic for you |
Indeed. I was thinking something like this: https://github.com/TDKorn/sphinx-github-style not sure whether it is the best option (we can check what numpy is doing exactly I guess) but something like this. RE linking yadism or EKO. Whenever the information is there, I'm happy with just linking it. But if it is not, better to leave porting the content for later (ideally, open an issue so that it gets assigned later!) |
that's better, but maybe still not the best way - which I would agree is the numpy way. It might be worth checking their implementation explicitly
NNPDF/yadism#386 - however, I would remove the current stuff since at best it is confusing (we're surely not doing it in Mellin space) and at worst it is just wrong |
|
@felixhekhorn @scarlehoff it looks like numpy is directly using sphinx.ext.linkcode (https://github.com/numpy/numpy/blob/main/doc/source/conf.py#L496C1-L598C1). I'll check what would be easiest for us |
if that works, that's just fine - but then we should take some heavy inspiration (a.k.a. copying) from what they do ... I was mostly worried that mapping would be complicated, but if we can just copy, that is fine |
I think roughly copying what they do should work. But I'm only wondering one thing, namely why do we care about doing all this if we have exclude_patterns = ['modules/**'] ( nnpdf/doc/sphinx/source/conf.py Line 78 in 4ae1a8c |
|
We are currently not linking the docs into the documentation. We want to start linking the docs, so it might be necessary to change |
|
@felixhekhorn @scarlehoff I implemented the changes in conf.py. I basically copied the numpy method. The only things I changed in their implementation are:
Then I tested it by asking codex to run it and provide me with a table that shows the modules and corresponding github links, which were the correct ones. Note two things:
Let me know if it looks ok to you! |
please test at least once yourself, i.e. build the documentation yourself and click on a link And this maybe will help you also to fix the action https://github.com/NNPDF/nnpdf/actions/runs/29324443681/job/87057256138?pr=2495 (which you still need to do of course) (I had a brief glance there but I don't know what actually is failing ...) |
Ok, will do! I think the failing action has something to do with the rust files that I adjusted, I'll fix it when I finish that! |
|
@felixhekhorn I'm finished making the changes. About testing the links, it's not possible to click a link in the generated documentation since they are currently not explicitly used inside the documentation (see comment by Juan). But I generated a test file containing one of the links, and this worked fine. |
I'm not sure I understand - why is it not working? which comment do you mean? are we not linking to our code? (in which case this whole issue and PR would be void, so I don't think that can be the case, can it?) or are the link targets not there? then we need to provide them of course ... |
That is also what I was wondering, if I understood @scarlehoff correctly we are now not linking in the docs, but at some point we want to and that's why we have to make this change. In any case I think nnpdf/doc/sphinx/source/conf.py Line 78 in 4ae1a8c makes sure that at the moment these modules are ignored by the makefile. Did I understand you correctly @scarlehoff ? |
|
At the moment what we have in the docs is this https://docs.nnpdf.science/_modules/index.html I haven't been able to follow the discussion so not sure what the issue is in the PR. But the issue to be solved is that one. |
do we have a link somewhere which actually points there @scarlehoff ? |
|
Ok, then I think we have to make some more changes as well. I.e. remove the modules from exclude_pattern and I think add some other things as well. Because as far as I understand, this sphinx viewcode makes this page, but linkcode (which I've implemented now) does not automatically (I think we have to make a dedicated rst file for it then) |
I think we removed it? It used to be in the main page. |
3 participants
No description provided.