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

feat: File inputs not able to be a single file #190

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

madhavmk
Copy link
Collaborator

@madhavmk madhavmk commented Nov 24, 2023

  • gather_files() returns dictionary of type Dict[str, List[str]]. all_files_dict dictionary values are now list of strings, even in case of single file value.
  • load_sound_speed() and load_deletions() modified to concatenate dataframes if multiple file input provided.
  • Unittests modified to support above changes.
  • tests/data/2022/NCL1/ctd/CTD_NCL_Ch_Mi, tests/data/2022/NCL2/ctd/CTD_NCL_Ch_Mi, tests/data/2022/NCL1/deletns.dat, tests/data/2022/NCL2/deletns.dat test files modified/added.
  • Fixes File inputs not able to be a single file #184
    • Ensures that "sound_speed", "travel_times", "gps_solution", "deletions" values can be a single-file value rather than a multi-file glob.
    • Now "sound_speed" and "deletions" accept multi-file values as input.

…_files_dict dictionary values are now list of strings, even in case of single file value.

* load_sound_speed() and load_deletions() modified to concatenate dataframes if multiple file input provided.
* Unittests modified to support above changes.
* tests/data/2022/NCL1/ctd/CTD_NCL_Ch_Mi, tests/data/2022/NCL2/ctd/CTD_NCL_Ch_Mi, tests/data/2022/NCL1/deletns.dat, tests/data/2022/NCL2/deletns.dat test files modified/added.
*
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1b2f5dc) 50.86% compared to head (61636cf) 50.98%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   50.86%   50.98%   +0.11%     
==========================================
  Files          16       16              
  Lines         863      865       +2     
==========================================
+ Hits          439      441       +2     
  Misses        424      424              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@madhavmk
Copy link
Collaborator Author

Hello @johnbdesanto,
The config.yaml now accepts both single-file and multi-file values for sound_speed, travel_times, gps_solution, and deletions:
e.g.

  input_files:
    sound_speed:
      path: ./tests/data/2022/**/ctd/CTD_NCL_Ch_Mi
    travel_times:
      path: ./tests/data/2022/NCL1/**/WG_*/pxp_tt
    gps_solution:
      path: ./tests/data/2022/NCL1/208/posfilter/POS_FREED_TRANS_TWTT
    deletions:
      path: ./tests/data/2022/NCL1/deletns.dat

I have performed tests with the sample test data. It would be really helpful if you could verify that this is also working with actual data and config on your system.
Thank you !

src/gnatss/loaders.py Outdated Show resolved Hide resolved
@lsetiawan lsetiawan removed the request for review from carlosgjs November 27, 2023 18:20
Copy link
Member

@lsetiawan lsetiawan left a comment

Choose a reason for hiding this comment

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

Thanks for these changes @madhavmk 😄 However, the new, duplicated test data created are not in the right structure.

  • NCL1 is the site ID that the data is captured from, therefor having NCL2 doesn't make sense and can be a source of confusion.
  • There are docstrings in code that doesn't make sense, please see my inline comment above.

If you can, please don't modify the test data directly, but rather you can do so on the fly.

* Revert pytest config.yaml to original configuration
@madhavmk
Copy link
Collaborator Author

Thanks for these changes @madhavmk 😄 However, the new, duplicated test data created are not in the right structure.

  • NCL1 is the site ID that the data is captured from, therefor having NCL2 doesn't make sense and can be a source of confusion.
  • There are docstrings in code that doesn't make sense, please see my inline comment above.

If you can, please don't modify the test data directly, but rather you can do so on the fly.

Thank you for reviewing the PR @lsetiawan .
I removed the duplicated docstring, and reverted the config.yaml its original configuration.

@madhavmk madhavmk requested a review from lsetiawan November 27, 2023 20:45
@lsetiawan lsetiawan merged commit 84ca950 into seafloor-geodesy:main Nov 27, 2023
5 checks passed
@madhavmk madhavmk deleted the issue_184 branch May 14, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File inputs not able to be a single file
2 participants