MDAnalysis / UserGuide

User Guide for MDAnalysis
https://userguide.mdanalysis.org
22 stars 31 forks source link

feat: Add TNG to the list of readers #275

Closed jandom closed 12 months ago

jandom commented 1 year ago

Maybe fixes https://github.com/MDAnalysis/UserGuide/issues/243 - could it be so simple?

hmacdope commented 1 year ago

You will also need to add a a page to reference the :ref: TNG against, see here: https://github.com/MDAnalysis/UserGuide/tree/6b82e9460e7b66ba742a17f2569842317b4cdf3b/doc/source/formats/reference

lilyminium commented 1 year ago

I think most .txt files are generated from scripts, this one being https://github.com/MDAnalysis/UserGuide/blob/develop/doc/source/scripts/gen_format_overview_classes.py .

There's also https://github.com/MDAnalysis/UserGuide/blob/develop/doc/source/scripts/core.py that needs to get updated.

lilyminium commented 1 year ago

Turns out scripts weren't running -- I opened #278 to fix this, but just warning that you might not see changes until that's in. If that's a blocker, one solution is to test locally by manually running the script itself.

jandom commented 1 year ago

Oh wow thanks for catching this @lilyminium, will wait – don't think this is pressing

jandom commented 1 year ago

@lilyminium not sure it helps but a single line change in core.py made this error go away and re-created a bunch of files. I think this (potentially?) goes hand-in-hand with your PR


Traceback (most recent call last):
  File "/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/scripts/gen_format_overview_classes.py", line 112, in <module>
    ov = FormatOverview()
  File "/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/scripts/base.py", line 36, in __init__
    self.get_lines(*args, **kwargs)
  File "/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/scripts/base.py", line 54, in get_lines
    lines.append(self.get_line(*items))
  File "/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/scripts/base.py", line 66, in get_line
    line.append(self._run_method(h, *args))
  File "/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/scripts/base.py", line 42, in _run_method
    val = meth(*args, **kwargs)
  File "/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/scripts/gen_format_overview_classes.py", line 55, in _description
    return DESCRIPTIONS[self.keys[-1]]
KeyError: 'TNG'
lilyminium commented 1 year ago

Yes, that's the key change that this PR needs to implement to fix the lack of TNG (the changes to the txt table should just get overwritten). It would add the TNG entry to the format overview table. However, if we have more to say about TNG, adding TNG.rst is necessary for the link to be functional. I tested the change you propose in this commit commit (https://github.com/MDAnalysis/UserGuide/pull/278/commits/687b9a8c2beaf198c47d230988364a4eda8b7000) in #278 -- it does indeed allow the docs building test to pass. (the examples test failed for unrelated reasons)

jandom commented 1 year ago

Okay this makes partial sense: there was some silent failure to generate some flies, because TNG was missing. I don't think I should be committing format_overview.txt or coordinate_readers.txt as these seem to be auto-generated?

Does anybody have a copy (the text we want to put into) the tng.rst file? Or should I just chat GPT their paper abstract into a 2-3 sentence summary?

lilyminium commented 1 year ago

@hmacdope do you have anything you'd like to say? These pages often document quirks, limitations, and assumptions made by the MDAnalysis parsers.

Or should I just chat GPT their paper abstract into a 2-3 sentence summary?

Otherwise this seems like a great alternative 😂

jandom commented 12 months ago

@hmacdope PLAL, i'm not sure what makes most sense to put in the TNG.rst, over what's already there in the MDA docs

jandom commented 12 months ago

@hmacdope bump, sorry to bug, PTAL, just because this is now blocking two PRs #280 and #271

hmacdope commented 12 months ago

@lilyminium given your comment in #280 I will let you decide when merging this is appropriate. My review cover the content. :)

jandom commented 12 months ago

Many thanks @hmacdope !

jandom commented 12 months ago

@lilyminium I've merged this one myself – this may have been a faux pas, because I don't know what's the merging custom. I found it quite surprising that others merged my PRs, but maybe that's totally normal here and I'm not supposed to merge (instead a maintainer is only allowed)

lilyminium commented 12 months ago

No worries at all @jandom, in the core library the maintainers often merge and I carried the habit over. Thanks for the fix, good to see CI green again!