Documentation overhaul#113
Conversation
Cinfdata db reader
Ms spectrum measurement
…tests for the ixdat native zilien parser
ScottSoren
left a comment
ScottSoren
left a comment
There was a problem hiding this comment.
Hi @AnnaWiniwarter ,
Thank you for doing this! This looks fantastic! I cloned your repository to build the documentations and read it all in .html while commenting.
Not all the comments require action, but some request some small changes, should be quick. I think it's high priority to merge this in, so anything that isn't quick should be done in a later PR. Yeah, I think it'll automatically build to readthedocs when we merge the PR.
The most commented parts are the tutorials. I take it you used a tool to convert jupyter notebooks to .rst? What tool did you use?
A problem is that the output blocks are indestinguishable from the code blocks. So hopefully we can modify the tool a bit to make them distinct.
| This can also be done straight from ``Measurement``, as follows: | ||
|
|
||
| >>> from ixdat import Measurement | ||
| >>> ec_meas = Measurement.get(3) |
There was a problem hiding this comment.
Do you actually use this?
I kind of consider it "not supported" until at least #75
But if it's (unexpectedly) useful, that's great :) . And the syntax of this line, at least, I expect will stay the same.
There was a problem hiding this comment.
No, before I read the documentation in detail I didn't even know about this possibility :) should we maybe delete this for now, then?
There was a problem hiding this comment.
It's fine to keep here. It works, and eventually it may be useful :)
| If you have any feedback, comments or questions, find out how to contact the ixdat team here: :ref:`questions`. | ||
| This documentation, like ``ixdat`` itself, is a work in progress and we appreciate any feedback or requests. | ||
|
|
||
| .. toctree:: |
There was a problem hiding this comment.
Nice! Huge improvement in the organization.
Hey, I wonder if we should also have a page linking to some of our slide decks on ixdat?
There was a problem hiding this comment.
I think that would be a great idea, but maybe not for this PR, so we can get this out asap?
Cinfdata db reader Fails linting. Need to fix.
|
@ScottSoren I have now addressed all the issues you had, except for the ones that are directly related to the EC-MS tutorial, as I feel this is really scope creeping here. I understand the things you mentioned (in some cases I have already created issues to fix the underlying problems in the ixdat methods), but I wonder if it's absolutely necessary to deal with all of these before merging this PR? I promise I will take care of them afterwards :) |
ScottSoren
left a comment
ScottSoren
left a comment
There was a problem hiding this comment.
Hey @AnnaWiniwarter ,
This looks good! As you point out, tutorials requires some discussion and will be addressed in another PR. We've left unresolved all the comments on the tutorial rst's here for reference at that time, and I've just made an Issue.
Github seems to have confused itself, so I'm seeing all the commits from #101 in the diff's, but hopefully the merge to main will go smooth. There's some small linting error which I'll fix after merging and make sure it passes tests.
Otherwise, amazing, important improvement here that will make ixdat a lot more usable and promotable. Hopefully readthedocs updates automatically when it merges (I will check). Thanks for the PR!
3 participants
This PR contains a complete overhaul of the readthedocs documentation page.
I tested the compiled website, but it would be a good idea if you could check that. Otherwise, I am not aware of which formatting requirements you have.
I ran invoke black, which lead to the zilien.py file being updated, not sure why that happened.
Sorry for the not-so-professional PR here.
Also, once merged, will the readthedocs page be updated automatically?