-
Notifications
You must be signed in to change notification settings - Fork 53
Adds documentation observation workflow #333
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
Conversation
docs/observations.rst
Outdated
@@ -0,0 +1,9 @@ | |||
Observations | |||
========== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be longer (same number of equals signs as characters in Observations
).
reading sources... [100%] tier1a
/home/xylar/code/mpas-work/analysis/adds_observational_xml/docs/index.rst:34: WARNING: toctree contains reference to nonexisting document 'mpascice'
/home/xylar/code/mpas-work/analysis/adds_observational_xml/docs/observations.rst:2: WARNING: Title underline too short.
Observations
==========
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think this will get us started.
A few questions emerge that may or may not be worth addressing in this PR:
- How would we specify multi-line entries if we wanted to?
- Would it be easy to modify the parser so it would only pull out those entries with a specific value for a specific tag (e.g.
component == 'ocean'
)? This would allow us to make separate tables from a single XML file. - How would we specify text plus URL for a proper hyperlink in the XML so we get the expected result in the table (i.e the text has a link to the URL rather than the URL being after the text)?
A few thoughts that I think we should address in a separate PR:
- I don't think division into Tier 1a, Tier 1b, Tier 2, etc. is useful outside of E3SM so I would just lump all the observations into one table per component instead.
- I don't think we want to store the table of observation in the docs, so I would move it to
mpas_analysis/obs
or something similar. We want it to be included in thempas_analysis
package. - We probably want to break the URLs out separately from the name of the data set. Also, we probably want a link directly to the data whenever available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do some PEP8 clean-up on the new script.
On second thought, it will be simple enough to have a separate XML file for each component. I don't see a downside to doing that. So no special parsing should be required. |
docs/tier1a.xml
Outdated
<observations | ||
headers="shortdesc, component, obsDataSet, references" | ||
headernames="Dataset, Component, Observational Dataset, References"> | ||
<aobs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "aobs" mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A OBServation"=aobs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, well that doesn't really work for me but it's something I can change in #335
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with "observation"
docs/tier1a.xml
Outdated
</detaileddesc> | ||
<obsDataSet> | ||
Merged Hadley Center-NOAA/OI data set from Hurrell et al. 2008 | ||
(https://climatedataguide.ucar.edu/climate-data/merged-hadley-noaaoi-sea-surface-temperature-sea-ice-concentration-hurrell-et-al-2008) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we put this inside <url>
and </url>
tags (without the parentheses)? Then, if the parser sees a url
tag, it would use the given text with the given URL as a hyperlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a version of the parser that handles <url>
tags:
https://github.com/xylar/MPAS-Analysis/blob/fill_in_obs_xml/docs/parse_table.py
(and has PEP8 fixes)
Here is the modified xml data including <url>
tags:
https://github.com/xylar/MPAS-Analysis/blob/fill_in_obs_xml/mpas_analysis/ocean/observations.xml
If you want, these changes can wait and I'll just make them in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this with Markdown url syntax instead: [name of url](http://url_link)
as I think this is clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can make all the changes I requested in a follow-up PR. I'd rather get this merged as it is so I can start working on that PR.
@milenaveneziani, if you don't have time to review this soon, I think that's fine. Just take yourself off as a reviewer and @pwolfram can merge the branch. If you'd like to review, feel free. |
@pwolfram, the problem with supporting Markdown urls in xml as in your latest commit is that the URL isn't easy to parse out of the XML, as we would want to do if we were automatically downloading the data set. We could parse out the URL using python string manipulation but that's a bit tricky. Anyway, certainly no harm in supporting this. |
Actually, I think the markdown support is great. It allows a lot of flexibility. If we specifically need to have a URL to download a dataset from, it will typically be different from the URLs we link to in the table (e.g. the exact file rather than a webpage with links to the data files). |
I'm working on an update here-- should be done soon. |
To give you a picture for where things might end up, My mock-up database looks something like:
|
930c8bc
to
8631b15
Compare
I have component-based tagging / splitting now |
@xylar, I'm happy for this to be merged now unless you see something I missed... |
Let me just re-test and then you can merge unless my test fails for some reason. |
@pwolfram, would it be easy to clean up the files to get rid of these warnings?
|
@xylar, there is a false positive following a nested list using tabulate for landice. This isn't worth the trouble to fix as it would require reparsing output of the tabulate library under particular rst rules. It is possible this is a soft bug with tabulate so if this is a big deal this an issue could be submitted to the library maintainers. |
2ff0654
to
94440f5
Compare
@pwolfram, don't worry about the last warning. It's better to leave it. We're going to change the table significantly from what you have here as an example in any case so maybe it just won't come up again. Go ahead and merge when you're ready. |
94440f5
to
2ff0654
Compare
How so? I think they are more or less orthogonal. Also, you approved #331, which suggested to me that it was ready to merge. |
I was able to rebase quickly with two very small conflicts to resolve. |
[title](http://example.com) and [title] (http://example.com) both now work
2ff0654
to
d462093
Compare
Thanks! My local clone got messed up... |
Rebase is ready to go now, thanks for checking in so we can get this resolved. |
Thanks! |
@pwolfram, I don't think this got merged in the desired way. It was a fast-forward merge. I don't think it's a big enough deal to edit the history but we want to make sure to avoid that in the future. The easiest way to do that is to use the merge button here on GitHub rather than the command-line tools. If you do use the command-line tools, make sure to always do merges with |
You are correct, I messed up and appreciate you correcting me here for the future. |
Adds xml file to keep track of documentation meta data and a script to parse this into a rst table for use in auto-generation of observation documentation.