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

updates to IMAP-Hi and SWE's document #283

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

tech3371
Copy link
Contributor

Change Summary

Before I forget, small PR for updates in document to include hi and minor updates to SWE.

New Files

  • docs/source/reference/hi.rst
    • add hi to docs

Updated Files

  • docs/source/reference/index.rst
    • update to include hi
  • docs/source/reference/swe.rst
    • updates SWE docs

@tech3371 tech3371 added Repo: Documentation Improvements or additions to documentation Ins: SWE Related to the SWE instrument Ins: Hi Related to the IMAP-Hi instrument labels Nov 21, 2023
@tech3371 tech3371 requested a review from a team November 21, 2023 00:20
@tech3371 tech3371 self-assigned this Nov 21, 2023
@tech3371 tech3371 requested review from bourque, sdhoyt, greglucas, bryan-harter, laspsandoval, GFMoraga and maxinelasp and removed request for a team November 21, 2023 00:20
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 think you'll need to update the references in the docstrings to be the fully qualified package name. np.array -> numpy.ndarray, xr.Dataset -> xarray.Dataset, Path -> pathlib.Path, I think you also have an extra _type_ in there too.


decom

IMAP-Hi test and validation code can be found below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally wouldn't worry about documenting the test and validation code, probably especially with the autosummary just because we don't generally put great docstrings into the tests. Maybe just a link to specific test or validation methods that would be useful to point others to (if any)?

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 didn't have anything to add for Hi at this moment besides those two. I could remove those and then we can add L1A as we develop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest removing them for now then. I actually think a description of the instrument and what it does is just fine for this start of documentation without any need to reference code here if there is nothing to reference. Just making the placeholder is a win :)


The L0 code to decommutate the CCSDS packet data can be found below.

.. currentmodule:: imap_processing
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be documented in the root level imap_processing location, not within hi.

l1a.swe_l1a
l1a.swe_science

The L1A code to process electron counts to rates and then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The L1A code to process electron counts to rates and then
The L1B code to process electron counts to rates and then

Out of curiosity are we capitalizing both "L" and "B" here, or are we calling them "L1b" or "l1b"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! If I start L with then it made sense to end with capital letter as well. Otherwise, not particular about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not particular about which version to choose, my only preference is for consistency if we can meet that. I think filenames will be lowercase l1b, but maybe if we are writing in sentences we will write L1B with all caps like you have?

l1a.swe_l1a
l1a.swe_science

The L1A code to process electron counts to rates and then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that you broke these up with a description of what they do too!

@@ -139,28 +139,28 @@ def calculate_calibration_factor(time):
3. Linear interpolate between those two nearest time and get factor for input time.

What this function is doing:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed that

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.

Looks great!

@tech3371 tech3371 merged commit 0213827 into IMAP-Science-Operations-Center:dev Nov 21, 2023
@bourque
Copy link
Collaborator

bourque commented Jan 31, 2024

@all-contributors please add @tech3371 for documentation.

Copy link
Contributor

@bourque

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

laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Apr 2, 2024
…#283)

* Added IMAP-Hi to the document and updated SWE's document
@tech3371 tech3371 deleted the hi_swe_doc branch July 25, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ins: Hi Related to the IMAP-Hi instrument Ins: SWE Related to the SWE instrument Repo: Documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants