Skip to content

Add time utils#3

Merged
Sceki merged 17 commits intokesslerlib:masterfrom
ANDREWNGT:Add_time_utils
Apr 4, 2022
Merged

Add time utils#3
Sceki merged 17 commits intokesslerlib:masterfrom
ANDREWNGT:Add_time_utils

Conversation

@ANDREWNGT
Copy link
Copy Markdown
Contributor

This pull request was motivated by the current cdm.py function being unable to take in day of year (DOY) date formats. It could only work with the traditional yy-mm-dd date formats. Since the CCSDS standards mandate that both formats can be used for CDMs, I have taken the liberty to translate the relevant MATLAB code in the NASA CARA analysis tools. https://github.com/nasa/CARA_Analysis_Tools/tree/master/two-dimension_Pc/Main/TransformationCode/TimeTransformations

I have also retrieved several test CDMs that were released by NASA and intend to add them here for easier reference.

Lastly, I have written a unit test for the time_utils functions that I have added.

Happy to take feedback as I am new to open source development workflows!

This commit adds time utility functions to enable the Kessler library to make use of CDMs with day of year formats instead of the traditional yy-mm-dd format.

This also adds a unit test function along with several test cdms retrieved from the NASA CARA repository.
@Sceki
Copy link
Copy Markdown
Member

Sceki commented Mar 18, 2022

Hi @ANDREWNGT , thank you for the PR, it is much appreciated!

Before I have a deeper look at the code (as soon as I find the time), I wanted to ask bring up two aspects:

  1. We still have to write a wiki with some coding style guidelines for the repo, and I am sorry for that. Anyway, I wanted to highlight that in Kessler we identify with Camelcase classes, and lower case with underscores all the methods and variables: could you please also do that here (since I see that you mostly use Camelcase)?

  2. We discourage pushing data that is not code to avoid unintended extra space being occupied in the user machine. Are you using all those CDM files that are pushed under the "input" folder in your unit tests? If not, I think you should not push them. If you are using them, I recommend using as little as possible to still satisfy the test, and having them directly into the test code (i.e., similar to here).

Thanks again!

Giacomo

@Sceki Sceki added the enhancement New feature or request label Mar 18, 2022
Copy link
Copy Markdown
Member

@Sceki Sceki left a comment

Choose a reason for hiding this comment

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

I have made some comments to review your code. Thank you for your contribution.

Also, please see the comment above, in order to be consistent with the library coding style and organization :)

Comment thread kessler/time_utils.py Outdated
Comment thread kessler/time_utils.py Outdated
Comment thread kessler/time_utils.py Outdated
Comment thread kessler/cdm.py Outdated
Comment thread kessler/time_utils.py Outdated
Comment thread kessler/time_utils.py Outdated
Comment thread tests/test_time_utils.py Outdated
@Sceki Sceki self-requested a review March 21, 2022 19:59
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 21, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@d3d5297). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master       #3   +/-   ##
=========================================
  Coverage          ?   11.85%           
=========================================
  Files             ?        6           
  Lines             ?      751           
  Branches          ?        0           
=========================================
  Hits              ?       89           
  Misses            ?      662           
  Partials          ?        0           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Sceki Sceki removed their request for review March 21, 2022 20:05
@ANDREWNGT ANDREWNGT requested a review from Sceki March 30, 2022 03:26
@Sceki
Copy link
Copy Markdown
Member

Sceki commented Mar 30, 2022

Thanks for the updates @ANDREWNGT !

Could you please also change from camel case to underscore (e.g. not thisVariable but this_variable), for all methods and variables ?

Comment thread kessler/cdm.py Outdated
Comment thread kessler/util.py Outdated
@ANDREWNGT ANDREWNGT requested a review from Sceki March 31, 2022 14:30
Comment thread kessler/cdm.py Outdated
Comment thread kessler/util.py Outdated
Comment thread kessler/util.py Outdated
Comment thread kessler/util.py Outdated
Padded date string with zeros in doy_2_date
@ANDREWNGT ANDREWNGT requested a review from Sceki April 2, 2022 14:20
@Sceki
Copy link
Copy Markdown
Member

Sceki commented Apr 4, 2022

This looks good to me, I am going to merge it, thanks @ANDREWNGT :)

@Sceki Sceki merged commit 3e062a5 into kesslerlib:master Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants