icesat2py / icepyx

Python tools for obtaining and working with ICESat-2 data
https://icepyx.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
210 stars 107 forks source link

Clean up APIformatting module and fix ATL11 temporal kwarg submission #515

Closed JessicaS11 closed 5 months ago

JessicaS11 commented 7 months ago

See #457, which results in an error if temporal keywords are submitted with a request for ATL11. This prompted #488, which it turns out having both CMR and EGI keywords submitted in the request is the desired behavior.

Tasks completed:

github-actions[bot] commented 7 months ago

Binder :point_left: Launch a binder notebook on this branch for commit a76aa05f5e947515e174cd92b60f3e9b4153f9db

I will automatically update this comment whenever this PR is modified

Binder :point_left: Launch a binder notebook on this branch for commit 825436efb72ac7b20196652aa5518d3828568b4f

Binder :point_left: Launch a binder notebook on this branch for commit c0519d7a123f9d607ee9e89ddbf5f7a1c8f0979f

Binder :point_left: Launch a binder notebook on this branch for commit e2820ba68ab8d3978bf7f788bc88af69da15127c

Binder :point_left: Launch a binder notebook on this branch for commit 05b53acddb530dd9d6491fcc83d0c3db68d3f05c

Binder :point_left: Launch a binder notebook on this branch for commit 37aa4475771307dea2314a21b2e4489e5da46420

Binder :point_left: Launch a binder notebook on this branch for commit 25e92d3f800bb2ff3cc7a67a694bb9f9a1a91564

Binder :point_left: Launch a binder notebook on this branch for commit fe74670aab404f96c03fb1889bb86107a38cc8dd

Binder :point_left: Launch a binder notebook on this branch for commit d262eda9eb7a21201f3059a0d0cb01a0474b02b6

Binder :point_left: Launch a binder notebook on this branch for commit 1e91bc4269e4dd2ee1d582713af2b04a2d39fdeb

Binder :point_left: Launch a binder notebook on this branch for commit 8f752cc1eb9cff1d67344dc5f8265e7fcacb59ce

Binder :point_left: Launch a binder notebook on this branch for commit ddf1ad2f5a530310d98d43cd7429140f08a8ae49

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 65.78947% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 66.59%. Comparing base (e5fd7d1) to head (ddf1ad2).

Files Patch % Lines
icepyx/core/APIformatting.py 64.28% 2 Missing and 3 partials :warning:
icepyx/core/granules.py 50.00% 4 Missing :warning:
icepyx/core/query.py 20.00% 2 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## development #515 +/- ## =============================================== + Coverage 66.19% 66.59% +0.39% =============================================== Files 36 36 Lines 3065 3059 -6 Branches 541 534 -7 =============================================== + Hits 2029 2037 +8 + Misses 945 934 -11 + Partials 91 88 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JessicaS11 commented 7 months ago

@whyjz What are the odds you'd be able to review this PR (or if not review the whole thing, since it has multiple changes, test it against your ATL11 workflow)? It would be greatly appreciated!

whyjz commented 6 months ago

@JessicaS11 Thanks a lot for this! I tested the orderurl branch with the following code:

reg_b = ipx.Query("ATL11", [-87, 74.7, -85, 75.4], ['2018-09-01','2022-01-01'])
reg_b.download_granules('./icepyx_test/')

Now it seemed to go through the order request (with the message showing Your order is: complete) but stopped immediately after that with this error message:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[5], line 1
----> 1 reg_b.download_granules('./icepyx_test/')

File ~/Projects/Github/icepyx/icepyx/core/query.py:1113, in Query.download_granules(self, path, verbose, subset, restart, **kwargs)
   1107     if (
   1108         not hasattr(self._granules, "orderIDs")
   1109         or len(self._granules.orderIDs) == 0
   1110     ):
   1111         self.order_granules(verbose=verbose, subset=subset, **kwargs)
-> 1113 self._granules.download(verbose, path, session=self.session, restart=restart)

TypeError: Granules.download() got an unexpected keyword argument 'session'

The same error also happens when I run the almost identical code to download ATL06:

reg_a = ipx.Query('ATL06',[-55, 68, -48, 71],['2019-02-20','2019-02-28']) 
reg_a.download_granules('./icepyx_test/')

This error does not appear when I use the code from the development branch. So I guess we solved the original issue, but the new changes also messed up something unexpectedly?

JessicaS11 commented 5 months ago

I am not sure how to resolve the Codecov insufficient coverage, though.

Unfortunately, the answer to that would be improving our test coverage (which I strive to do, but isn't always practical). In this case, what's happening is Codecov checks to see if the lines of code modified in the PR are tested and compares the percent tested of those lines to the percent tested of the software (overall) on the development branch. If the line of code that was modified did not already have a test covering it, it brings down the "patch" coverage percentage and thus more likely to fail this check. There's not a great way to fix this without having the modified lines of code tested to start with, and my code testing skills are novice enough to make this a nontrivial task. The Codecov checks are still really helpful though for finding some of the lower lift ways to improve test coverage, even if they don't always pass.

Thanks so much for the PR review!