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

New tools documentation #163

Merged
merged 6 commits into from
Sep 26, 2023
Merged

New tools documentation #163

merged 6 commits into from
Sep 26, 2023

Conversation

GFMoraga
Copy link
Contributor

@GFMoraga GFMoraga commented Sep 21, 2023

Change Summary

Made a new tools directory in docs/source/reference/tools . This will have documentation on how to use the tools that are created.

PLEASE give feedback on .rst formatting and what to add/not add.

  • how do I do the index (tree?) attachment if I need to.

This should check off a "to-do" in #75

New Files

  • xtce_generator.rst
    • documentation on how to use the xtce_generator_template.py to create instrument specific scripts

@GFMoraga GFMoraga added Repo: Documentation Improvements or additions to documentation Ins: CoDICE Related to the CoDICE instrument Level: L0 Level 0 processing labels Sep 21, 2023
@GFMoraga GFMoraga self-assigned this Sep 21, 2023
@GFMoraga GFMoraga marked this pull request as ready for review September 22, 2023 15:23
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I'm not sure where we want to put this, but if we want to keep it in the API reference section.


Adding another heading below instruments would be a good spot for it I think.

I am somewhat hesitant to put it in the API reference though because it isn't a part of the installed package. Maybe we want this to be up in the development section instead and then add it to the toc over there?

Comment on lines 21 to 26
Before you begin, ensure you have the following:

- Python installed (version 3.9 or higher)
- The required Python packages installed:
- pathlib
- tools.xtce_generation.telemetry_generator
Copy link
Collaborator

Choose a reason for hiding this comment

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

pathlib is a part of the standard library, and I don't think you need to install telemetry_generator but rather you just use it right?

I'd suggest indicating how you do this with the poetry environments and linking to those documents as well. Something similar to this, feel free to reword as you want.

Generating XTCEs is only done whenever packet definitions get updated, and thus it is not a part of the main processing package. To use it there are a few extra dependencies like pandas that you can install with poetry install --with-extras xtce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this!

docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

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

The page looks good overall, I added a few suggestions. Only real changes needed are moving the XML files into another PR, and adding the document to a toctree so it can be discovered.

@@ -3,42 +3,42 @@
<xtce:Header date="2023-09" version="1.0" author="IMAP SDC" />
Copy link
Contributor

Choose a reason for hiding this comment

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

These XML files should be in a different PR, they aren't related to the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxinelasp how do I take these "out" of this commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that we can do that through git reset soft command but I will let @maxinelasp or @greglucas chime in because I struggle with those kinds of things myself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're still OK here Gabriel, but it won't be super easy/obvious how to update it either. I think what happened is:

  1. You updated the XML generator
  2. You then ran the generator and updated the codice XML files
  3. You committed both 1 and 2
  4. You got feedback to remove 2 and tried to git rm *.xml in that directory
  5. Now those files are removed and thus have been updated in the opposite direction and still show changes

If that is right, then I think we want to just get back to point 1 above and commit that update?

The way I would approach that is by trying to get back to the original work you had and then modifying that commit. Here is a list of commands that I ran to accomplish this and push to my remote so you can see what it'd look like:
dev...greglucas:imap_processing:gml-xtce-update-docs

Note that during the rebase, I changed the first and third commit to "edit" from "pick" so that I could stop and modify those commits and remove the files from staging. I don't think that stuff is captured here, so these commands won't be verbatim what you need. Also notice the liberal use of git status and git log so I can see where I am in the process and what the current commit has in it.

10088  gh pr checkout 163
10089  git status
10090  git log
10091  git checkout -b gml-xtce-update-docs
10092  git fetch upstream
10093  git rebase -i upstream/dev
10094  git status
10095  git reset --soft HEAD~1
10096  git status
10097  git restore --staged imap_processing/codice/packet_definitions/*.xml
10098  git status
10099  git checkout imap_processing/codice/packet_definitions/*.xml
10100  git status
10101  git log
10102  git commit
10103  git rebase --continue
10104  git status
10105  git restore --staged imap_processing/codice/packet_definitions/*.xml
10106  git status
10107  git commit
10108  git status
10109  git rebase --continue
10110  git status
10111  git push -u origin gml-xtce-update-docs
10112  git log

docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/reference/tools/xtce_generator.rst Outdated Show resolved Hide resolved
@GFMoraga GFMoraga changed the title New tools documentation and updated xtce files New tools documentation Sep 25, 2023
Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

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

Looks a lot better, nice! Just those two code blocks to update from me, and please take the xml files out of the review.

docs/source/development/tools/xtce_generator.rst Outdated Show resolved Hide resolved
docs/source/development/tools/xtce_generator.rst Outdated Show resolved Hide resolved
Copy link
Contributor Author

@GFMoraga GFMoraga left a comment

Choose a reason for hiding this comment

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

I just deleted the xtce files and will readd in new PR.

@GFMoraga GFMoraga requested a review from maxinelasp September 25, 2023 21:31
Copy link
Contributor

@tech3371 tech3371 left a comment

Choose a reason for hiding this comment

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

looks good

@GFMoraga
Copy link
Contributor Author

GFMoraga commented Sep 25, 2023 via email

Copy link
Collaborator

@bourque bourque left a comment

Choose a reason for hiding this comment

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

Nice work @GFMoraga! Looks good!

@GFMoraga GFMoraga dismissed maxinelasp’s stale review September 26, 2023 21:41

I fixed all! The right way!

@GFMoraga
Copy link
Contributor Author

@greglucas I rebased! Thank you for the command guidance.

Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

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

This looks good to me! Nice job. There is one warning to fix, I put in a comment so you can just click the button to update it.

Minor change to reduce warning

Co-authored-by: Maxine Hartnett <117409426+maxinelasp@users.noreply.github.com>
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Nice work! I'm glad you were able to parse the cryptic rebase, it is never easy to recover from things like that.

docs/source/development/tools/index.rst Show resolved Hide resolved
@GFMoraga GFMoraga merged commit 0865c82 into IMAP-Science-Operations-Center:dev Sep 26, 2023
12 checks passed
@GFMoraga GFMoraga deleted the xtce-update-docs branch September 26, 2023 22:13
maxinelasp pushed a commit to maxinelasp/imap_processing that referenced this pull request Sep 29, 2023
* Made xtce documentation in tools and added files with toctree. Had to rebase.
maxinelasp pushed a commit to maxinelasp/imap_processing that referenced this pull request Oct 2, 2023
* Made xtce documentation in tools and added files with toctree. Had to rebase.
laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Nov 15, 2023
* Made xtce documentation in tools and added files with toctree. Had to rebase.
@bourque
Copy link
Collaborator

bourque commented Jan 31, 2024

@all-contributors please add @GFMoraga for documentation

Copy link
Contributor

@bourque

I've put up a pull request to add @GFMoraga! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ins: CoDICE Related to the CoDICE instrument Level: L0 Level 0 processing Repo: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants